-
Notifications
You must be signed in to change notification settings - Fork 9
Create python package for linter #39
base: main
Are you sure you want to change the base?
Conversation
@PhilippWendler Could you please review? |
Co-authored-by: Philipp Wendler <[email protected]>
Is this to be assigned to @SvenUmbricht? |
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.
Is there anything specific that developers of the linter need to be aware of? That should be documented. I guess at least the fact that the project uses Poetry is such a fact.
@PhilippWendler Can we merge this? From my side the PR is ready, do you have anything else to add? |
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
__version__ = "1.4" |
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.
Why does this PR still change the version number instead of simply keeping it at 1.3-dev
?
And now, if we do not rewrite the history of this PR, we will have several different commits in the project history where the linter will claim to be version 1.4, with potentially different behavior (if version 1.4 is actually released at some point in the future, because the next version should be 1.3?). This is exactly what should be avoided by bumping the version only directly before and after a release, as I suggested before.
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 was under the impression that we wanted to release the package immediately once this PR is merged. But I did forget that one could now find this version number on multiple commits with different behaviors. Then again, the same is true for the version numbers we used so far, so this does not worsen the situation much. We can publish the first release as version 1.5, and from then on only bump the version directly before/after new releases to avoid this problem in the future.
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 would leave out versions 1.3 and 1.4 then and change this PR to set the version to 1.5-dev, then once the PR is merged we can do the release by removing the -dev, publishing, then bumping to 1.6-dev
Rationale: going back to 1.3 will be confusing with the commits here containing 1.4; If we leave out 1.3, we might as well leave out 1.4 as well, there is no reason not to jump to 1.5-dev here.
This PR is for @MartinSpiessl to review and accept, I am just providing some comments. I am also not doing a full review (e.g., I did not actually install Poetry and try everything out, which would of course be needed for a proper review), just telling you when I notice something. And I have no experience with Poetry. |
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.
Very nice PR! I added some comments where things can further be improved in my opinion.
You can use either your username/password combination or an API token for authentication. | ||
Please refer to `https://python-poetry.org/docs/repositories/#configuring-credentials` for details. | ||
|
||
4. Create and push a git tag for the new release. |
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.
maybe it makes sense to do this step (4. pushing the tag) together with (6. bumping to next development version) directly after / when doing 1. . This allows the person performing the release to push all 3 changes (release commit, tag, post-release dev version commit) in one go.
Then 2., 3., and 5. can be done on the release tag without the risk of someone accidentally pushing in the meanwhile (the risk for this is slim, but the proposed change of order also makes it better structured I think)
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.
Yes, that seems sensible. It might also make sense to perform step 2 (build and test the relesase archive) first, otherwise we might notice something still missing from the release after pushing the release version/tag.
So the suggestion would be to change the order to 2, 1, 4, 6, 3, 5
.
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.
We would only push stuff in 4, 1 I picture as just doing the changes locally
So the suggestion would be to change the order to 2, 1, 4, 6, 3, 5.
I would bump to the new version (6) before pushing (4), such that we only need to push once, so that would be:
2, 1, 6, 4, 3, 5.
Here we would need to rebuild (2) after doing 1, so as outlined at the beginning of this post I still think it should be:
1, 2, 6, 4, 3, 5.
Steps 1,2,6 are local changes and builds, if something goes wrong this can be rewritten. 4 would then push the two commits and the tag to github and make the version official. 3 and 5 is then about publishing the actual release archives. Does that make sense? I think the unclarity stems from the fact that the current description of 1 does not specify whether these changes are done locally or already pushed.
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.
Of course we would only push once, in the proposed order 2, 1, 4, 6, 3, 5
step 4
would only be creating the tag, and pushing would be done in step 6. I still think that step 4
should be done before step 6
, as otherwise we have already changed the version number to the next development version when creating the tag.
But I agree that step 2
should be done after 1
after all to avoid having to rebuild for the release. As long as this is done before the push I have no objections. So can we agree on the following:
1, 2, 4, 6, 7, 3, 5
where 7
is the push, and 4
is only creating the tag?
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
__version__ = "1.4" |
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 would leave out versions 1.3 and 1.4 then and change this PR to set the version to 1.5-dev, then once the PR is merged we can do the release by removing the -dev, publishing, then bumping to 1.6-dev
Rationale: going back to 1.3 will be confusing with the commits here containing 1.4; If we leave out 1.3, we might as well leave out 1.4 as well, there is no reason not to jump to 1.5-dev here.
Regarding poetry: I looked into it and found it to be quite nice (apart from the terrible installation procedure, cf. #39 (comment)), so I would say we can use it for handling packaging etc. |
This PR intends to address #27.
I have never distributed a package before, so feedback is welcome.