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

[WIP]Bug fixing attempts for GSoC 2024 #4530

Closed
wants to merge 2 commits into from

Conversation

SampurnaM
Copy link
Contributor

@SampurnaM SampurnaM commented Mar 25, 2024

Attempts to fix #2879

Changes made in this Pull Request:

  • Commented which functions I would like to change in groups.py , the functions in topologyattributes.py are failing because of their dependencies on objects/check in groups.py

I have never worked with such a well-organized, large code base, so I have commented out things as I am afraid to mess things up. Happy to delete this branch or submit a fresh PR with code.

PR Checklist

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4530.org.readthedocs.build/en/4530/

@pep8speaks
Copy link

Hello @SampurnaM! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 150:1: W293 blank line contains whitespace
Line 354:1: E265 block comment should start with '# '
Line 355:1: E265 block comment should start with '# '
Line 356:1: W293 blank line contains whitespace
Line 358:38: E261 at least two spaces before inline comment
Line 358:39: E262 inline comment should start with '# '
Line 361:65: E261 at least two spaces before inline comment
Line 361:66: E262 inline comment should start with '# '
Line 361:80: E501 line too long (93 > 79 characters)
Line 387:1: E265 block comment should start with '# '
Line 388:1: W293 blank line contains whitespace
Line 557:1: W293 blank line contains whitespace
Line 563:1: W293 blank line contains whitespace
Line 565:1: W293 blank line contains whitespace
Line 567:14: E114 indentation is not a multiple of four (comment)
Line 567:14: E303 too many blank lines (2)
Line 607:8: E114 indentation is not a multiple of four (comment)
Line 608:13: E265 block comment should start with '# '
Line 611:13: E265 block comment should start with '# '
Line 613:14: E114 indentation is not a multiple of four (comment)
Line 613:14: E117 over-indented (comment)
Line 614:14: E114 indentation is not a multiple of four (comment)
Line 614:14: E117 over-indented (comment)
Line 614:80: E501 line too long (106 > 79 characters)
Line 614:107: W291 trailing whitespace
Line 615:13: E265 block comment should start with '# '
Line 621:17: E265 block comment should start with '# '
Line 622:17: E265 block comment should start with '# '
Line 623:21: E117 over-indented (comment)
Line 623:21: E265 block comment should start with '# '
Line 626:18: E261 at least two spaces before inline comment
Line 626:19: E262 inline comment should start with '# '
Line 635:9: E266 too many leading '#' for block comment
Line 643:13: E265 block comment should start with '# '
Line 644:13: E265 block comment should start with '# '
Line 645:17: E265 block comment should start with '# '
Line 646:1: W293 blank line contains whitespace

Line 864:25: W291 trailing whitespace
Line 868:1: W293 blank line contains whitespace
Line 869:80: E501 line too long (93 > 79 characters)
Line 870:9: E265 block comment should start with '# '

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

Copy link

Linter Bot Results:

Hi @SampurnaM! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ✅ Passed

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/8416471988/job/23043195524


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@SampurnaM SampurnaM changed the title Bug fixing attempts for GSoC 2024 [WIP]Bug fixing attempts for GSoC 2024 Mar 25, 2024
@orbeckst
Copy link
Member

As a general rule, don't comment out code when you want something to be reviewed. We tend to look at the diff https://github.com/MDAnalysis/mdanalysis/pull/4530/files and we just want to see old code (red) and new code (green).

As long as you have "WIP" in the title or the PR is in "draft status", people will likely not look at it. Most developers are too busy to review drafts. Sometimes a draft PR emerges from a discussion along the lines of "This sounds like an interesting idea, why don't you make a quick draft PR so that we see what the idea looks like concretely in code and then we discuss there."

@orbeckst
Copy link
Member

Furthermore, I strongly suggest you change the title to something that immediately conveys what is being fixed so that it catches the eyes of developers that care about the specific thing. The original issue title is generally a good starting point.

It's very good that you're linking the original issue. (You don't have to include "attempts" or "hopefully" — just "fixes" is fine. We all strive to do things right and sometimes it's harder than we think, that's understood by everyone here.)

@SampurnaM
Copy link
Contributor Author

"We all strive to do things right and sometimes it's harder than we think, that's understood by everyone here." - you just made my day.

I am closing this PR ( there was a great PR ahead of me on this issue) and focus on [(https://github.com//pull/4531)]. Thanks :)

@SampurnaM SampurnaM closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atomnames methods should handle empty group
3 participants