-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Migrate to tox-dev and v4 support #74
Comments
I've made a POC version at https://github.com/gaborbernat/tox-gh-actions/blob/support-v4/src/tox_gh_actions/plugin.py#L1-L108 (depends on tox-dev/tox#2191) 👍 |
Thanks for proposing this! Let me have a look at the document and think about it.
I think we can consider migration to tox-dev and tox v4 support separately but I'm 100% agree on supporting tox-gh-actions from the day 1 of tox v4. I didn't know tox v4 is getting ready, I'm glad to hear that 🙂 I'll prioritize tox v4 support and will have a look at your PoC soon (like this weekend).
Yep, will do that. |
My POC is passing CI and all that so would recommend we use that as a working base 😊 but is not entirely feature full, I'm happy to discuss the gaps and changes needed from tox core to get there. However, before I move past the POC I wanted your eyes on it. |
Thanks for the PoC :) I could get ideas on how to use tox v4's APIs, although the PoC includes many changes irrelevant to the tox v4 support itself and the PoC's logic to override the envlist is different from tox v3. Considering that I decided to create a new branch and implement tox v4 support incrementally. https://github.com/ymyzk/tox-gh-actions/tree/tox4 It's mostly working locally and I plan to push the update to this branch soon. |
I strongly disagree. The way you had implemented it in v3 is just not supported in tox v4, and you'll want my poc for things to make sense and be supported. |
Could you elaborate a bit more on which part is not supported and how you want to fix that? I'm trying not to break the current behavior as much as possible to make it easier for users to migrate from tox v3. This is one of the highest priorities to me. |
Could you please elaborate on irrelevant changes? |
Sure. This commit contains https://github.com/gaborbernat/tox-gh-actions/commit/a39cab4b4753cad523e4a1e5d965aeb441d23050 a lot of irrelevant changes for adding tox v4 support to this plugin. Do we really need to use your coding style for supporting tox v4? |
I don't think I've added any irrelevant changes, so you'll have to clearly name each of these changes to have a conversation about it. I'm not sure which stylistic changes you're referring to, but each is inline with what core tox does. So following the same style should make easier for people to contribute to both. 🤷♂️ |
If we want to make styling changes, I would prefer to do it in a separate issue. The styling change commit itself is large and has different issues (e.g. adding Sphinx related metadata shouldn't be necessary for this plugin). It's making it difficult for me to review the changes. If we want to really prepare tox v4 support soon, I feel we should focus on what we really need. I don't want to discuss further about the styling issue in this issue. By the way, considering the experience in this ticket, I'm leaning towards not to migrate the project under tox-dev at this point. |
Sure 👍 you're free to do as you wish, if you don't want to explain in detail you don't have to. Good luck! 🍀 I'll note here that I might add a few other changes and release my interpretation of what a GitHub actions tox plugin should be. |
Hello, would you consider moving the project under the tox-dev umbrella? See documentation under https://tox.readthedocs.io/en/rewrite/plugins.html#adoption-of-a-plugin-under-tox-dev-github-organization
Furthermore, tox v4 is getting ready and we'd like to make sure this plugin is supported from day 1, we're collecting feature gaps for this under tox-dev/tox#1974. Would be great if you could join our development chat under https://discord.gg/tox so we can assist with this. If you do so please drop in a line in the
#plugin
chat with the name of the repository you maintain. Thanks!The text was updated successfully, but these errors were encountered: