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

Move to keyword only arguments to improve readability #94

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

julianstirling
Copy link
Collaborator

New pylint allows you to seperately set a maximum number of total aguments and a maximum number of positional arguments. This gave us lots of warnings on the number of functions that had a confusingly high number of arguments.

Rather than just up the limit in pylintrc it seems like good practice to make many of these arguments keyword only. This make code more clear, and more robust as we go forward.

@julianstirling
Copy link
Collaborator Author

This is in draft until we merge #93 as we need to fix the setup imports for the CI to function

New pylint allows you to seperately set a maximum number of total aguments
and a maximum number of positional arguments. This gave us lots of warnings
on the number of functions that had a confusingly high number of arguments.

Rather than just up the limit in pylintrc it seems like good practice to make
many of these arguments keyword only. This make code more clear, and more robust
as we go forward.
@julianstirling julianstirling marked this pull request as ready for review October 21, 2024 13:21
@julianstirling
Copy link
Collaborator Author

Now #93 is merged we can get this in.

Copy link
Contributor

@jmwright jmwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julianstirling Thanks for your work on this. I have noted files where I expect we'll end up with merge conflicts, but we'll just have to fix them when the time comes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may end up with a merge conflict on this file since I've been working in it a lot on my other-shelves branch. I'll deal with it if it happens, but I expect it will.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as with fasteners.py. We'll probably end up with merge conflicts.

@jmwright jmwright mentioned this pull request Oct 21, 2024
@julianstirling julianstirling merged commit fa0d083 into master Oct 24, 2024
5 checks passed
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.

2 participants