8000 Use python-versioneer to support __version__ by BenjaminRodenberg · Pull Request #70 · precice/python-bindings · GitHub
[go: up one dir, main page]
Skip to content

Use python-versioneer to support __version__ #70

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Nov 20, 2020
Merged

Use python-versioneer to support __version__ #70

merged 15 commits into from
Nov 20, 2020

Conversation

BenjaminRodenberg
Copy link
Member

This PR restructures the structure of this repository in the following way:

  • cyprecice provides the cython extension
  • test provides (as before) the test
  • precice provides a pure python package

This restructuring was necessary, since I could not find a nice way to implement support for __version__ in a cython-based package. However, for a python based package (which precice now is) there is https://github.com/python-versioneer/python-versioneer, which comes with some additional features.

Closes #50

@BenjaminRodenberg BenjaminRodenberg added bug Something isn't working enhancement New feature or request labels Nov 19, 2020
@BenjaminRodenberg BenjaminRodenberg self-assigned this Nov 19, 2020
ajaust
ajaust reviewed Nov 19, 2020
View reviewed changes
Copy link
Collaborator
@ajaust ajaust left a comment

Choose a reason for hiding this comment

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

I have added some comments here. I think my biggest concern is the complexity of the solution. It seems like a lot of changes and code (including new dependencies) for getting the __version__ property.

@BenjaminRodenberg
Copy link
Member Author

I think my biggest concern is the complexity of the solution.

@ajaust I totally agree. To me it also seems a bit too much just for getting __version__. But I could not find a simple solution for directly adding __version__ in the cython package. Some ideas to make this PR a bit less overwhelming:

  • I could move the "restructuring" into a different PR. Basically 449abab + some minor things. I think that this is in any case a step into the right direction, since this makes the repo a bit more tidy.
  • I think we cannot avoid introducing the wrapper python package (I could not find a pure cython way)
  • We could avoid using the versioneer. There are also more minimalistic approaches (see https://stackoverflow.com/a/7071358/5158031). Even though I think that the versioneer has some nice features and will also help us to streamline the release cycle.

Any opinions on the suggestions above?

@ajaust
Copy link
Collaborator
ajaust commented Nov 19, 2020

@BenjaminRueth I am much more supporting of the changes now some more explanations regarding versioneer and the additional changes. My biggest fear to have a new (build) dependency just for this feature, but since it is not needed and since you are convinced of versioneer I would suggest to stick with it. Especially since you see further potential for streamlining processes due to versioneer. If we become unhappy with the situation, we can still remove versioneer and go for a different approach.

So I am fine with using versioneer and the restructuring. Whether you prefer to do the restructuring here or in a different PR I leave to you. I think you know better what to do than I do.

@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Nov 20, 2020
Merged
BenjaminRodenberg added a commit to ajaust/python-bindings that referenced this pull request Nov 20, 2020
@BenjaminRodenberg
Make packaging optional for warnings, add note on precice#70
73451b8
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Nov 20, 2020
Merged
@BenjaminRodenberg
Copy link
Member Author

So I am fine with using versioneer and the restructuring. Whether you prefer to do the restructuring here or in a different PR I leave to you. I think you know better what to do than I do.

I'm doing the restructuring in #71. After that we can merge #70.

@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Nov 20, 2020
Merged
1 task
IshaanDesai
IshaanDesai approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member
@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

The solution looks quite complicated but purely from the user perspective the bindings work as expected.

@BenjaminRodenberg
Copy link
Member Author

I'm done now with moving all the non-related changes out of this PR (#71 and f5de714). The diff should be much clearer now. I will merge this PR into develop soon.

@BenjaminRodenberg BenjaminRodenberg merged commit 7d2c705 into develop Nov 20, 2020
@BenjaminRodenberg BenjaminRodenberg deleted the fix_pkg branch November 20, 2020 15:53
BenjaminRodenberg added a commit that referenced this pull request Nov 21, 2020
@ajaust @BenjaminRodenberg
Patching setup.py for installation without pip and packaging for Spack (
daf8aea 
#63)

* Improve readability and check uses_pip, before warnings
* Require pip >= 19.0.0, if installed via pip.
* Update documentation. pip3 19.0.0 is minimum requirement. Remove troubleshooting for unsupported pip versions.
* Make command in Exception consistent with README.md
* Make packaging optional for warnings, add note on #70
* Update CHANGELOG.md

Co-authored-by: Benjamin Rüth <benjamin.rueth@tum.de>
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Feb 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajaust ajaust ajaust left review comments

@IshaanDesai IshaanDesai IshaanDesai approved these changes

Assignees

@BenjaminRodenberg BenjaminRodenberg

Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support __version__
3 participants
@BenjaminRodenberg @ajaust @IshaanDesai
0