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

Switch to poetry and rename to aiomqtt #210

Merged
merged 8 commits into from
Jun 12, 2023
Merged

Switch to poetry and rename to aiomqtt #210

merged 8 commits into from
Jun 12, 2023

Conversation

empicano
Copy link
Owner

@empicano empicano commented May 1, 2023

Finally found some time for this! @frederikaalund, @JonathanPlasse, if you could look over the pyproject.toml that would be great, in particular, I'm not sure if I configured the poetry-dynamic-versioning right.

I published it on Test PyPI: https://test.pypi.org/project/aiomqtt-test/0.16.1.post59

In this PR I still want to update all references from asyncio-mqtt to aiomqtt in the documentation and the code. When the PR is merged I would release 0.17.0 both under the aiomqtt and the asyncio-mqtt names. For asyncio-mqtt I would add a log warning explaining the rename and the migration and publish it manually. Then I would change the name of the repository on GitHub (which also updates the URL of the documentation).

Contrary to what I proposed in #202 I would make this the last release for asyncio-mqtt. The rename doesn't break anything for asyncio-mqtt users that want to stay with the old name, and the migration should be straightforward. Also, I admit that I don't know how to publish to two names automatically and expect this to be unnecessarily complex.

Let me know what you think! 😊

@JonathanPlasse
Copy link
Collaborator

I am busy this week.
It will take some time for me to review this PR.

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@frederikaalund frederikaalund left a comment

Choose a reason for hiding this comment

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

First of all, thank you Felix for taking your time to look into this! :)

Overall it looks great. 👍 I only have some minor comments. Feel free to ask questions. I'll try to elaborate in a timely manner.

I agree with your asyncio-mqtt to aiomqtt rename strategy. 👍 Again, let me know if you need assistance with that. :)


Optional: Prefer "standard" version scheme of PEP 440 for the dependency specifications (e.g., no carets and tildes). PEP 440 is part of PEP 621 and the latter aims to standardize the dependency specification across project management/build systems (poetry, hatch, setuptools, etc.). Might as well hop on board already. :)

.python-version Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@empicano empicano force-pushed the poetry-and-rename branch from 8837502 to bbf6273 Compare May 3, 2023 14:54
@empicano
Copy link
Owner Author

empicano commented May 3, 2023

I'd like to make our CONTRIBUTING.md a bit easier by going for GitHub's "Scripts to rule them all". We can then also use these scripts inside the GitHub workflows (see scripts/setup inside the docs workflow). What do you think about that?

@empicano
Copy link
Owner Author

empicano commented May 3, 2023

Does any of you have experience with using poetry-dynamic-versioning inside the GitHub workflow and can point me to a code snippet? I'm a bit stuck as to how to do this. I also found this GitHub action that takes the version from the tag on GitHub: https://github.com/marketplace/actions/pypi-poetry-publish

@frederikaalund
Copy link
Collaborator

I'd like to make our CONTRIBUTING.md a bit easier by going for GitHub's "Scripts to rule them all". We can then also use these scripts inside the GitHub workflows (see scripts/setup inside the docs workflow). What do you think about that?

Great idea! 👍 That is one of the more difficult things to maintain manually. PRs usually forget about it or find that its a barrier for contribution.

Does any of you have experience with using poetry-dynamic-versioning inside the GitHub workflow and can point me to a code snippet?

I'm afraid not. I tried it way back (in another project) but never got a workflow up around it.

@empicano
Copy link
Owner Author

@JonathanPlasse what does this JUnit stuff here and in the test workflow do? Do we still need this?

@JonathanPlasse
Copy link
Collaborator

Yes, we still need this.
It is used to have error messages in the PR code where there are mypy or pytest errors.

@empicano
Copy link
Owner Author

pre-commit is really starting to annoy me, I just want to test the workflows without pulling in the changes from main

@JonathanPlasse
Copy link
Collaborator

You can use workflow_dispatch to manually run the workflows.

@frederikaalund
Copy link
Collaborator

Great to see progress on this issue. 👍 Keep up the good work! :)

I have some time the next couple of days to contribute as well. Let me know if you need something from my side.

@empicano empicano force-pushed the poetry-and-rename branch from 6f211dc to bb7903c Compare May 19, 2023 14:13
@empicano
Copy link
Owner Author

You can use workflow_dispatch to manually run the workflows.

Maybe I'm not seeing something, but I can only run the workflows from main there.

I'm a bit frustrated with changing the tooling to poetry. This takes a lot more time than I anticipated. Can we remove pre-commit in favor of a script (see scripts/check that runs ruff and mypy) that we can run locally and that runs on each push and PR? I feel that pre-commit is really slowing me down (e.g. see above, there's no reason this should block the other workflows, but also I want to run the checks when I want, not necessarily on each commit).

Related, the idea to "enable the strict rules and then help contributors fix them" from #217 doesn't work if pre-commit forces contributors to fix things before they commit.

If we remove pre-commit I think I can finish this PR with some oversight from Jonathan on the JUnit stuff. Otherwise, I have to pass this on to someone else because I'm missing the time. Independently, I can still update the docs to the new name.

@frederikaalund
Copy link
Collaborator

It's fine with me to disable pre-commit to get the move to poetry done. 👍 If I had to choose between the two, then poetry wins no doubt. :) We have all the checks server-side (via GitHub workflows) anyhow.

The idea with pre-commit is that you run the same checks locally first to avoid the wait time (i.e., the wait time for the GitHub workflows to complete). :) That is a nice-to-have feature. We can disable it for now to get things moving.

On a side note: Now that we have a poetry-based local environment, we can use that for pre-commit. That is, use repo: local in the .pre-commit-config.yaml for all python-based "hooks". See an example of that type of config here: https://github.com/sbtinstruments/cyto/blob/master/.pre-commit-config.yaml
This way, we make pyproject.toml file for all our python dependencies (even dev dependencies). Otherwise, pre-commit makes separate envs for it's hooks (and different versions).

@frederikaalund
Copy link
Collaborator

Also, there is always git commit --no-verify if you wan to skip the pre-commit hooks. They are optional in that sense. We can't enforce the client to apply said hooks. :)

@empicano
Copy link
Owner Author

there is always git commit --no-verify if you wan to skip the pre-commit hooks.

That's a good tip, thanks 🙂 We should definitely add that to the CONTRIBUTING

The idea with pre-commit is that you run the same checks locally first to avoid the wait time

Right, that's a good thing, I just prefer to run those checks when I want it, and not have them forced on me when I want to commit 😄 Anyways, if I interpret it right, you're for keeping pre-commit as a workflow (which is what is blocking me from testing the updated workflows atm), so I'll focus on replacing all occurrences of "asyncio-mqtt" with "aiomqtt" in the docs, and let someone else finish the move to poetry 😋

@frederikaalund
Copy link
Collaborator

Anyways, if I interpret it right, you're for keeping pre-commit as a workflow (which is what is blocking me from testing the updated workflows atm), so I'll focus on replacing all occurrences of "asyncio-mqtt" with "aiomqtt" in the docs, and let someone else finish the move to poetry 😋

Ah, I actually didn't realize that we use pre-commit as a driver for our GitHub workflows. 😅

Well, let's get the rename done with. Then we'll tackle the move the poetry in another PR. :) 👍

@frederikaalund
Copy link
Collaborator

I just looked the GitHub workflow files. I can't see any reference to pre-commit there. Am I missing something?

@empicano
Copy link
Owner Author

empicano commented May 19, 2023

Screenshot 2023-05-19 at 21 23 21

that we use pre-commit as a driver for our GitHub workflows.

Yes, and who knows why, but pre-commit blocks the execution of other workflows because there are merge commits (even though this is still a draft). I can't look into the project settings, but I suspect that pre-commit is listed as an authorized app there and that this is how it has access.

@JonathanPlasse
Copy link
Collaborator

This is a GitHub app which must be disabled.

@JonathanPlasse
Copy link
Collaborator

You should be able to disable it Here

@empicano
Copy link
Owner Author

empicano commented Jun 5, 2023

Ping @frederikaalund in case this got lost 😉

@frederikaalund
Copy link
Collaborator

Ping @frederikaalund in case this got lost 😉

Sorr about that! 😅 It did get lost among my other work.

I just suspended the pre-commit app. Let me know if you need something more! 👍

@empicano empicano force-pushed the poetry-and-rename branch 7 times, most recently from f73c05d to 0a8bfdf Compare June 9, 2023 16:11
@empicano
Copy link
Owner Author

empicano commented Jun 9, 2023

@JonathanPlasse, any idea why the PyPy tests keep failing? The other tests work fine, and I can't wrap my head around the Directory /opt/hostedtoolcache/PyPy/3.9.16/x64/lib/pypy3.9 for cffi does not seem to be a Python package error.

(Solved by pinning the snok/install-poetry action to its highest version.)

@empicano empicano force-pushed the poetry-and-rename branch 5 times, most recently from 60451bc to 7a0e253 Compare June 12, 2023 16:49
@empicano empicano force-pushed the poetry-and-rename branch from 7a0e253 to 54ebb37 Compare June 12, 2023 16:59
@empicano
Copy link
Owner Author

This is finally finished! @frederikaalund, can you rename the repository on GitHub? I've updated all the links and mentions to the new name, that's the only thing missing.

@mossblaser, thank you again for generously liberating the aiomqtt name! 🎉 😎

@empicano empicano merged commit 5442902 into main Jun 12, 2023
@empicano empicano deleted the poetry-and-rename branch June 12, 2023 17:15
@frederikaalund
Copy link
Collaborator

This is finally finished! @frederikaalund, can you rename the repository on GitHub?

Done!

Thank you @empicano for (yet another) great contribution to asyncio-mqtt! 😄👍

And thanks to @mossblaser for the aiomqtt name. :) Makes me nostalgic, because mossblaser's aiomqtt was my original motivation for asyncio-mqtt.

@mossblaser
Copy link

mossblaser commented Jun 13, 2023 via email

@frederikaalund
Copy link
Collaborator

I'd welcome a suitable message to be inserted into my old aiomqtt repo's readme pointing people to your project if you'd be happy to suggest some suitable words?

I already like the current You might prefer aiomqtt section. :) Thank you for that, btw.

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

Successfully merging this pull request may close these issues.

4 participants