-
Notifications
You must be signed in to change notification settings - Fork 34
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
Use the same config file as Tox #59
Conversation
@giginet: Are you up for giving this a try, to see if works for you? |
Hi @ryanhiebert - I have a few comments and was looking at this yesterday, but got pulled away. Just letting you know that I've got feedback incoming. |
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.
Largest issue for me is the code duplication is parser_cfg
#60 tries to address this.
src/tox_travis/utils.py
Outdated
return config._cfg | ||
|
||
|
||
def get_section(section, cfg): |
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.
This seems to be unused code?
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.
Looks like it. I thought that I'd be using this all over, but when I went through, no other place seemed like it would benefit from the code churn, so I abandoned it.
src/tox_travis/hooks.py
Outdated
|
||
override_ignore_outcome(config) | ||
override_ignore_outcome(config, cfg) |
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.
config
and cfg
are very similar names - might be confusing to future contributors. Considering that cfg
is accessible as config._cfg
, it'd make sense to always pass the config
, then access _cfg
as needed.
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 agree that it's confusing. The competing goal which motivated using cfg
instead of config
is that 1) almost nothing needs the full config, and 2) config._cfg
is obviously an implementation detail of config
, and I'd rather not scatter unstable APIs everywhere. Still it's confusing, and I can concede that there's not enough places we have to work with it that it's too big a deal.
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.
config._cfg
is obviously an implementation detail ofconfig
, and I'd rather not scatter unstable APIs everywhere.
True, what makes sense to me would be to
- hide the internal API in a utility method
- only pass config, and call the utility where necessary
something like:
# utils.py
def get_iniconfig(config):
return config._cfg
# wherever the IniConfig is needed
tox_ini = get_iniconfig(config)
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'd be happy with that.
src/tox_travis/after.py
Outdated
"""Determine if this job should wait for the others.""" | ||
config = py.iniconfig.IniConfig('tox.ini') | ||
section = config.sections.get('travis:after', {}) | ||
section = cfg.sections.get('travis:after', {}) |
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 don't particularly like passing the tox config all the way down the call stack, but it seems kind of necessary in the current design. Other options would be to:
- set a module-level global that
after_config_matches
accesses directly. - create a
ToxAfter
class that takes the config in it's init, then refactor the various module functions as methods, andafter_config_matches
accesses theconfig
directly on the instance.
But I'm not convinced that these options are a clear improvement. Just ideas.
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 agree that's its a bit annoying to pass the config down the stack, but I also agree that I'm not convinced that the other options are clear improvements at this point. If we had other global(ish) state that we needed, then ToxAfter
(and probably a matching ToxEnv
) would probably be my preferred approach. As it is, we'd probably just end up trading config
in several places with self
everywhere and extra indentation.
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.
As it is, we'd probably just end up trading config in several places with self everywhere and extra indentation.
exactly.
src/tox_travis/hooks.py
Outdated
from .toxenv import default_toxenv, override_ignore_outcome | ||
from .after import travis_after_monkeypatch | ||
|
||
|
||
@tox.hookimpl | ||
@tox.hookimpl(trylast=True) |
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.
Given that the PR parses the ini anyway, is this necessary?
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.
Yep, as you already discovered.
src/tox_travis/utils.py
Outdated
Figure out the configfile that Tox is going to use, and | ||
create a ``py.iniconfig.IniConfig`` from that file, or | ||
``None`` if no configuration file could be found. | ||
""" |
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.
Your docs are always great... 🙌
I was wondering if this would be superceded by #61, but clearly it's based on this one. I'm going to clean this up and merge it. I'm not going to do a release yet, since there seems to be more stuff still in progress. |
Tests? |
Yes, those too. I spoke like I wasn't going to, but I definitely do, and hopefully a green review too ;-) |
619e6ff
to
14b1717
Compare
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
=========================================
- Coverage 75.4% 73.3% -2.11%
=========================================
Files 4 4
Lines 187 206 +19
Branches 46 51 +5
=========================================
+ Hits 141 151 +10
- Misses 38 43 +5
- Partials 8 12 +4
Continue to review full report at Codecov.
|
src/tox_travis/hooks.py
Outdated
|
||
override_ignore_outcome(config) | ||
override_ignore_outcome(config, cfg) |
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.
For this one, why not get the boolean unignore_outcomes
here, and pass that to override_ignore_outcome
as an arg.
I'd be tempted to do the same with travis_after
, passing a Section
or env_requirements
list object down, so that the ini parsing is done here, which will make ini config errors happen earlier in the tox sequence.
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.
Way late, but FWIW I'm definitely considering this approach for a possible refactor. It mostly depends just how involved I need to get into the config. For unignore_outcomes, it's pretty simple. For toxenv, it's unfortunately a bit more complicated bacause of a backward compatibility concern. But it seems like a good approach in general.
14b1717
to
df2fed4
Compare
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 77.22% 77.27% +0.04%
==========================================
Files 5 5
Lines 202 198 -4
Branches 46 46
==========================================
- Hits 156 153 -3
+ Misses 39 38 -1
Partials 7 7
Continue to review full report at Codecov.
|
df2fed4
to
13ce1b2
Compare
13ce1b2
to
f4375ab
Compare
This is ready for review. I've added a test, and rebased it to be based on us now using the tox_configure hook. So it's just using the config that Tox loaded, instead of loading our own. |
Confession: I totally wanted to skip writing tests. My excuse was that it was a bug fix and I wanted to get it fixed quickly. In hindsight the original pull request was certainly too big to skip tests, and I'm really thankful to @rpkilby for calling me out on it with a single word. I apologize for hiding my true intentions at the time. At this point the patch is much smaller, and has added a regression test. In an effort to maintain my own momentum, I may merge some PRs that I perceive to be less challenging, like this one now is, though I intend to give @rpkilby a couple days before I do that with this particular pull request. Or anyone else that's willing. I sometimes take @rpkilby for granted because of how absolutely awesome he's been as a maintainer. |
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.
👍
If successful, this fixes #57.
No new tests have been added, though some have been modified to adjust to the new call signature.