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

Don't raise conflicting basepython warnings when -e option has been used to target a different environment #1426

Conversation

prashantmital
Copy link

First-time contributor alert!

Closes #1124

This only runs the basepython conflict check if the current environment is amongst the ones requested to be targeted by the user via the -e option.

Thanks for contributing a pull request!

If you are contributing for the first time or provide a trivial fix don't worry too
much about the checklist - we will help you get started.

Contribution checklist:

(also see CONTRIBUTING.rst for details)

  • wrote descriptive pull request text
  • added/updated test(s)
  • updated/extended the documentation
  • added relevant issue keyword
    in message body
  • added news fragment in changelog folder
    • fragment name: <issue number>.<type>.rst for example (588.bugfix.rst)
    • <type> is must be one of bugfix, feature, deprecation,breaking, doc, misc
    • if PR has no issue: consider creating one first or change it to the PR number after creating the PR
    • "sign" fragment with "by :user:<your username>"
    • please use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files - by :user:superuser."
    • also see examples
  • added yourself to CONTRIBUTORS (preserving alphabetical order)

@asottile
Copy link
Contributor

seems fine, mind adding a quick test and a changelog entry? can probably copy the other basepython conflict test, expand it to have a second testenv that doesn't have a conflict and assert the output there

I think this'll also be magicallyfixed by the rewrite that @gaborbernat is working on (via lazily-evaluated environment configurations)

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Add a changelog entry and it's okay. Granted tox 4 will fix this, but that's probably few months down the line for now due to its overall complexity and lack to time on my side, so no need to hold progress until then.

@gaborbernat
Copy link
Member

Ideally we would want a test too, for validation.

@prashantmital
Copy link
Author

I'll work on the test and update the ticket in a few days.

@gaborbernat
Copy link
Member

@prashantmital still interested in pushing this over the line?

@prashantmital
Copy link
Author

Sorry @gaborbernat I just don't have the bandwidth to finish this up. Its also not a high-priority issue since the error message is purely cosmetic and will be resolved by the rewrite that was mentioned at some point in this thread.

Thanks for your work in maintaining this project. Cheers!

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.

Conflicting basepython check fails for factor+testenv combination
3 participants