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

Library uses short argument names #78

Closed
tekktrik opened this issue Nov 8, 2022 · 6 comments · Fixed by #82
Closed

Library uses short argument names #78

tekktrik opened this issue Nov 8, 2022 · 6 comments · Fixed by #82
Labels
enhancement New feature or request

Comments

@tekktrik
Copy link
Member

tekktrik commented Nov 8, 2022

pylint suggests using argument names with at least 3 letters. This library uses argument names of shorter length, and while these warnings have been disabled for now, they should be considered for renaming. This may require the rework of Learn Guides and other references to code snippets.

@tekktrik tekktrik added the enhancement New feature or request label Nov 8, 2022
@makermelissa
Copy link
Collaborator

I'm not aware of any learn guides using this, but it would make sense to add _pin to most of the names.

@makermelissa
Copy link
Collaborator

When I added the V2 support, I added _pin where it wouldn't break compatibility, so most of this is now done.

@tekktrik
Copy link
Member Author

Does it make sense to close this now then?

@DJDevon3
Copy link

or now with 9.0 just released? Seems most of this was cleaned up by Melissa in this commit a long time ago.

@tekktrik
Copy link
Member Author

The only remaining disables I see are here:

cs: Optional[Pin] = None, # pylint: disable=invalid-name
dc: Optional[Pin] = None, # pylint: disable=invalid-name

@makermelissa are these compatibility breaking? Not sure if you had used examples or Learn Guides to benchmark that. I'm also fine with say this is good for now if changing these couple isn't advisable.

It's also worth mentioning that with the current discussion around switching to the ruff linter and formatter, this would also be affected (or possibly ignored).

@makermelissa
Copy link
Collaborator

@makermelissa are these compatibility breaking? Not sure if you had used examples or Learn Guides to benchmark that. I'm also fine with say this is good for now if changing these couple isn't advisable.

Possibly. It depends on whether somebody has used these as a keyword argument. I'm not aware of any code I made that did theat, but I can't vouch for other folks.

@tekktrik tekktrik linked a pull request Mar 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants