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

updating style doc for list parameter #200

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

robotjellyzone
Copy link

Fixes #181

Fix done: Updated the user guide of contribution code to adjust new style following type hints

@robotjellyzone
Copy link
Author

robotjellyzone commented Apr 15, 2022

@lilyminium @micaela-matta can you please once check this pr and let me know if any changes are needed:)

@lilyminium
Copy link
Member

lilyminium commented Apr 15, 2022

Thanks for opening this PR, @robotjellyzone. I see that you've added in type hinting into the example -- could you please also add some text explicitly requesting that people follow type hinting recommendations?

And, ah... @PicoCentauri, did we settle on a style for type hinting? It seems to me that we will have to stick with old style List[int] for a while yet. Even the linked PEP 484 https://peps.python.org/pep-0484/ shows that syntax.

@robotjellyzone
Copy link
Author

Thanks for opening this PR, @robotjellyzone. I see that you've added in type hinting into the example -- could you please also add some text explicitly requesting that people follow type hinting recommendations?

And, ah... @PicoCentauri, did we settle on a style for type hinting? It seems to me that we will have to stick with old style List[int] for a while yet. Even the linked PEP 484 https://peps.python.org/pep-0484/ shows that syntax.

@lilyminium i have added the note requesting to follow type hints. please check & let me know for any further changes:)

@PicoCentauri
Copy link

And, ah... @PicoCentauri, did we settle on a style for type hinting? It seems to me that we will have to stick with old style List[int] for a while yet. Even the linked PEP 484 https://peps.python.org/pep-0484/ shows that syntax.

list[int] only works for python versions larger or equal to 3.9. With this, we have to stick to the old syntax (List[int]) for actual type hinting until we support older versions. In the docs however only list[int] links everything nicely. I have no clue if we should use different syntax in the docs and the type hint? Regardless of our decision, we should make this very clear in the userGuide!

Copy link
Contributor

@micaela-matta micaela-matta left a comment

Choose a reason for hiding this comment

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

Thanks!

@robotjellyzone
Copy link
Author

And @lilyminium can you please check this pr?

@micaela-matta
Copy link
Contributor

And @lilyminium can you please check this pr?

Please be patient and stop repeatedly tagging. Be respectful of our free labor: It’s a public holiday weekend and we do this in our free time. Your request is not urgent and will be dealt with when possible.

@robotjellyzone
Copy link
Author

robotjellyzone commented Apr 18, 2022

And @lilyminium can you please check this pr?

Please be patient and stop repeatedly tagging. Be respectful of our free labor: It’s a public holiday weekend and we do this in our free time. Your request is not urgent and will be dealt with when possible.

Oh so sorry for that!! Yeah actually I knew that I should not tag again but actually you set the deadline yesterday that after 17th no pr will be considered so that's why it was a bit stressing. I hope you can understand and I didn't actually mean to disturb 🙏🏻.
.
I myself have been a mentor and I know its stressing sometimes but when it's deadline then everyone hurries which I know very well but still sorry if it hurt you in anyway!

@micaela-matta
Copy link
Contributor

Don’t worry, the warning was mainly meant for people just starting new PRs not for the ones that are mostly done!

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

Thanks for adding the note, @robotjellyzone. Unfortunately as hinted in the question I asked here: #200 (comment), the style is actually a little more nuanced than that, so it warrants a bit more detail. I've suggested what I think is the best temporary solution below. @MDAnalysis/coredevs if you have different ideas, please comment :)

doc/source/contributing_code.rst Outdated Show resolved Hide resolved
@lilyminium
Copy link
Member

Also please add yourself to AUTHORS here as well :)

@orbeckst
Copy link
Member

@lilyminium this is a pretty old PR. Is it still relevant or should we close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants