Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made method names consistent and added docstrings #24

Merged
merged 2 commits into from
May 8, 2023

Conversation

BSculfor
Copy link
Contributor

@BSculfor BSculfor commented May 5, 2023

Closes Issue #12. I know most of the method names are self-documenting, but it doesn't hurt to have docstrings anyway. The second commit changes the names in test_main.

P.S. I'd like to be able to make some more substantive changes, but I don't have a bluesky account. If you have a spare invite I'd appreciate it.

BSculfor added 2 commits May 5, 2023 21:17
IK most of the method names are self-documenting, but it doesn't hurt to have docstrings anyway. Also think it's better to have consistent names (and that was one of the open issues) even if it means a slight divergence from the API; the whole point is that the python user doesn't need to know about the API!
update the test files to reflect changes made in previous commit
@ianklatzco
Copy link
Owner

Amazing, thank you!

Would you be so kind as to change them to thisCase so they're consistent with the case in atproto? I'll send 2 invites your way if so ^^

@BSculfor
Copy link
Contributor Author

BSculfor commented May 6, 2023

I'd figured that snake_case was better because it fits python conventions, even if it diverges from atproto. Part of the point of this project (as I see it) is that the user shouldn't need to know the details of atproto, and it's easy enough to work out the API method name from the python method name in these cases. All of the Session methods are python methods, but only some correspond to distinct API methods, so it makes sense to me to stick to python conventions for all of them. Perhaps use snake_case for the Session methods and camelCase for the methods of the proposed APIAction class?

Ultimately it's your project though, and I can definitely see why it would be desirable. While I'm doing this, should I make some of the other names closer to the atproto names (e.g. get_skyline to getTimeline)?

@ianklatzco
Copy link
Owner

all good considerations!

i was hoping this library could be friendly / easy-to-use. there's now https://github.com/marshalX/atproto which is a more-full-fledged library, so i think the role of this one will be "utility functions", "REPL for your skeets", "teaches you the names+cases of the xrpc endpoints", "friendly", and "easy to use".

to that end i think consistent camelCase across the board is probably best, if you'd be so kind 🙏

@ianklatzco ianklatzco merged commit fcba754 into ianklatzco:master May 8, 2023
@ianklatzco
Copy link
Owner

made the changes myself! thanks for motivating me ^^

shoot me an email - iklatzco AT gmail - for a pair of invite codes ^^

@ntindle
Copy link

ntindle commented May 10, 2023

I know I have no say here, but I'd like to voice that following the language guidelines is probably a good idea, as stated in pep-0008 https://peps.python.org/pep-0008/#method-names-and-instance-variables

Lots of tools will throw a fit if this isn't matched.

@ianklatzco
Copy link
Owner

Hmmm that's quite a tempting consideration, thanks for sharing -- PEP8 is pretty canonical, right?

I suspect https://github.com/marshalX/atproto is probably pep8 and worth being canonical - i think i intend for atprototools to serve more of the "getting started as quick as possible" wrapper library ^^

@davidcarryer
Copy link

PEP8 is pretty solid. Worth consideration. Building on a standard, especially a Python Standard, makes good sense.

@BSculfor
Copy link
Contributor Author

I know I have no say here, but I'd like to voice that following the language guidelines is probably a good idea, as stated in pep-0008 https://peps.python.org/pep-0008/#method-names-and-instance-variables

Yes, PEP8 was what I was referring to when I said "snake_case fits python conventions" in my earlier comment, it's The Standard for python. All the auto-formatting tools (including autopep8 that you recommended using when contributing to the project) are minor variations of PEP8 (usually just having less strict limits on line length).

I stand by my earlier suggestion that we could have something like a APIAction or RawSession class which follows the AT protocol/xrpc closely, so:

  • camelCase names
  • Instances/methods that mirror xrpc methods exactly (i.e. createRecord/deleteRecord rather than rebloot/deleteBloot)
  • methods that all return a requests.Response

We'd keep the Session class as a more usable utility version that:

  • Uses snake_case
  • methods that more closely reflect a user rather than developer experience (e.g. rebloot and delete_bloot methods)
  • methods that return the content of interest rather than the raw requests.Response

Then RawSession would be used by people who really want to grapple with AT protocol, and the Session can be used by people who just want to write a bot or do something quick. The details would still need to be sketched out, e.g. do we have an APIAction class where the xrpc methods are instances, or a RawSession class where xrpc methods are methods, but this presents a compromise between teaching Lexicon/AT protocol/xrpc and having a pythonic interface (in both naming and usage).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants