Skip to content

style: convert variables to lowercase #159

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

Merged
merged 6 commits into from
Jun 21, 2025

Conversation

john-halloran
Copy link

This is a mostly complete conversion of variables to lowercase. Have tested to make sure behavior remains the same with before renaming. I'm still working on a few minor variables. The cleanup of old files is no longer included in this PR.

@john-halloran john-halloran changed the title chore: convert variables to lowercase style: convert variables to lowercase Jun 19, 2025
Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

this is sooo much better. Please see my comments. Mostly on the docstrings, which are way more easily understood now.

When these things are fixed I am inclined to merge this, but we probably should revisit it. In general, we would prefer to write tests before we do the decsign and the functions, but this may not be a good strategy here. @john-halloran what are your thoughts on this.

@john-halloran
Copy link
Author

john-halloran commented Jun 19, 2025

this is sooo much better. Please see my comments. Mostly on the docstrings, which are way more easily understood now.

When these things are fixed I am inclined to merge this, but we probably should revisit it. In general, we would prefer to write tests before we do the decsign and the functions, but this may not be a good strategy here. @john-halloran what are your thoughts on this.

I think for understandability, it will be good to get this renaming out of the way asap, but otherwise I agree with all of this. Tests will be the next thing I work on and that will happen before any further development on the algorithms, to avoid our results drifting from the known-working state they are in currently.

@john-halloran
Copy link
Author

Everything should be renamed now, so if you see anything still capitalized, flag that in your review. Feel free to merge once all your feedback is addressed. Thanks for the help so far!

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

really good. Just one or two things and I can merge

@john-halloran
Copy link
Author

Okay, that stuff should be in. Let me know if I had the right idea.

@sbillinge sbillinge merged commit 27ea989 into diffpy:john-development Jun 21, 2025
1 check passed
@sbillinge
Copy link
Contributor

@john-halloran nailed it! nicely done. Code is 1000% more readable!

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