-
Notifications
You must be signed in to change notification settings - Fork 6
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
Comply with PEP518 #119
Comply with PEP518 #119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies it's a very very quick review (it's quite late on my end 😅 ), but here are some initial thoughts.
Overall this looks great, there's a few things I would advise changing, the big ones (e.g. versioningit) are very much lightweight suggestions.
Main thing that would need changing here is the dynamic
entry for version
.
Co-authored-by: Irfan Alibay <[email protected]>
* Switch to versioningit with updated method in RTD.
Oops not sure what happened with the tests, sorry! I'll try to have a stab at fixing up whatever I messed up a bit later today. |
No worries, thanks for your help here. I think it's just a missing package for the metadata when running tests. I'll give a try. |
@ojeda-e looks like the package name internally is "MembraneCurvature" instead of "membrane_curvature" is that intended? |
I was checking that line exactly. I think there was always a messy naming here with all the variations of "membrane" + "curvature" and the names are inconsistent across the package. :/ |
Co-authored-by: Irfan Alibay <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So overall from my end, i.e. "code wise", this looks good so I'm going to approve. However, I don't have a clue about the whole authors & maintainers stuff.
I think it's something to be discussed once a few of the coredevs come back from their holidays.
Sounds good, thanks for your help and pointers here @IAlibay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MDAnalysis devs can't promise maintaining the kit. Therefore we need to find another name/email for maintainers
.
If you're looking for a new maintainer then I would
- put your own name back in maintainer
- add a conspicuous note in README and in the docs saying that you're looking for a maintainer and how to contact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy with the changes.
(Sorry, I don't know if/how AUTHORS files work in the authors variable, I just made a random guess.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IAlibay would please help me with reviewing AUTHORS? Hopefully I didn't miss anything there,
Quickly scanning through there was one PR by a now deleted Github user - just docstring changes.
I'd say the authors file looks good to me!
Description
Fixes #108 and #104
Add
pyproject,toml
to comply with PEP518. Deleted unnecessary files:Status