Skip to content
This repository has been archived by the owner on Oct 14, 2022. It is now read-only.

Create python package for linter #39

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Create python package for linter #39

wants to merge 13 commits into from

Conversation

SvenUmbricht
Copy link
Contributor

This PR intends to address #27.
I have never distributed a package before, so feedback is welcome.

@dbeyer
Copy link
Member

dbeyer commented Sep 25, 2021

@PhilippWendler Could you please review?

lint/witnesslinter.py Show resolved Hide resolved
lint/witnesslint/__init__.py Outdated Show resolved Hide resolved
lint/pyproject.toml Outdated Show resolved Hide resolved
lint/setup.cfg Outdated Show resolved Hide resolved
lint/setup.cfg Outdated Show resolved Hide resolved
Co-authored-by: Philipp Wendler <[email protected]>
@PhilippWendler
Copy link
Member

Is this to be assigned to @SvenUmbricht?

Copy link
Member

@PhilippWendler PhilippWendler left a 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.

lint/witnesslint/linter.py Outdated Show resolved Hide resolved
lint/pyproject.toml Outdated Show resolved Hide resolved
@SvenUmbricht SvenUmbricht marked this pull request as ready for review February 3, 2022 13:24
@SvenUmbricht
Copy link
Contributor Author

@PhilippWendler Can we merge this? From my side the PR is ready, do you have anything else to add?

lint/doc/Developing.md Outdated Show resolved Hide resolved
#
# SPDX-License-Identifier: Apache-2.0

__version__ = "1.4"
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@PhilippWendler
Copy link
Member

@PhilippWendler Can we merge this? From my side the PR is ready, do you have anything else to add?

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.

Copy link
Collaborator

@MartinSpiessl MartinSpiessl left a 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.

lint/doc/Developing.md Show resolved Hide resolved
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.
Copy link
Collaborator

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)

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

lint/witnesslinter.py Show resolved Hide resolved
#
# SPDX-License-Identifier: Apache-2.0

__version__ = "1.4"
Copy link
Collaborator

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.

lint/doc/Changelog.md Show resolved Hide resolved
@MartinSpiessl
Copy link
Collaborator

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.
If we encounter any problems in the future, we can still switch to something else. If not we might use it in other projects as well.

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

Successfully merging this pull request may close these issues.

5 participants