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

Snake case #500

Merged
merged 3 commits into from
Oct 18, 2020
Merged

Snake case #500

merged 3 commits into from
Oct 18, 2020

Conversation

jsfan
Copy link
Contributor

@jsfan jsfan commented Oct 17, 2020

Following on from #446 this PR transforms some camelCased names to snake_case.

This does not close #144 because there are many more occurrences of camelCase which should really be snake_case.

@jsfan
Copy link
Contributor Author

jsfan commented Oct 17, 2020

it probably makes no sense to clean up all variable and method names in one PR because that would lead to a very large patch which would be impossible to review.

@shelld3v Does this look good to you so far? Feel free to modify in my repo again.

@shelld3v
Copy link
Collaborator

Why? I don't understand why you do this, most of variables I created when updating dirsearch are something like camelCased

@jsfan
Copy link
Contributor Author

jsfan commented Oct 17, 2020

@jsfan
Copy link
Contributor Author

jsfan commented Oct 17, 2020

It's not something flake8 checks but it is part of PEP8.

@shelld3v
Copy link
Collaborator

I still don't think it's a good idea

@jsfan
Copy link
Contributor Author

jsfan commented Oct 18, 2020

You don't have to go with it. The whole point of PEP8 though is that there are shared guidelines. So, if you were to find a contributor who is used to Python guidelines, they'd likely use snake case just as a Java coder would likely use camel case. Right now, the code base has not fixed style and my conclusion would be to go with what is generally adopted in the language it is written in. But, once again, it's your code base, so I'm only making a suggestion.

@jsfan
Copy link
Contributor Author

jsfan commented Oct 18, 2020

@maurosoria I've resolved the conflicts.

@shelld3v
Copy link
Collaborator

I actually don't like this PR, but I'm not the owner

@maurosoria maurosoria merged commit 0818da4 into maurosoria:master Oct 18, 2020
@maurosoria
Copy link
Owner

Hello @shelld3v and @jsfan

I wanted to do this changes by myself, but I never have the opportunity (because it has to be done progressively). What @jsfan is pointing out is totally reasonably: if this is not done, PEP8 makes no sense.

Thanks @jsfan and @shelld3v !

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.

Need help for PEP8
3 participants