Use the same config file as Tox #59

ryanhiebert · 2017-03-01T06:44:33Z

If successful, this fixes #57.

No new tests have been added, though some have been modified to adjust to the new call signature.

ryanhiebert · 2017-03-01T13:58:04Z

@giginet: Are you up for giving this a try, to see if works for you?

rpkilby · 2017-03-02T16:37:00Z

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.

rpkilby

Largest issue for me is the code duplication is parser_cfg #60 tries to address this.

rpkilby · 2017-03-01T17:32:46Z

src/tox_travis/utils.py

+    return config._cfg
+
+
+def get_section(section, cfg):


This seems to be unused code?

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.

rpkilby · 2017-03-02T21:38:04Z

src/tox_travis/hooks.py


-    override_ignore_outcome(config)
+    override_ignore_outcome(config, cfg)


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.

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.

config._cfg is obviously an implementation detail of config, 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)

I'd be happy with that.

rpkilby · 2017-03-02T22:22:12Z

src/tox_travis/after.py

    """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', {})


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, and after_config_matches accesses the config directly on the instance.

But I'm not convinced that these options are a clear improvement. Just ideas.

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.

As it is, we'd probably just end up trading config in several places with self everywhere and extra indentation.

exactly.

rpkilby · 2017-03-02T22:25:17Z

src/tox_travis/hooks.py

 from .toxenv import default_toxenv, override_ignore_outcome
 from .after import travis_after_monkeypatch


-@tox.hookimpl
+@tox.hookimpl(trylast=True)


Given that the PR parses the ini anyway, is this necessary?

Yep, as you already discovered.

rpkilby · 2017-03-02T22:29:46Z

src/tox_travis/utils.py

+    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.
+    """


Your docs are always great... 🙌

ryanhiebert · 2017-03-02T23:46:00Z

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.

rpkilby · 2017-03-02T23:52:54Z

I'm going to clean this up and merge it.

Tests?

ryanhiebert · 2017-03-03T00:02:23Z

Yes, those too. I spoke like I wasn't going to, but I definitely do, and hopefully a green review too ;-)

codecov · 2017-03-10T06:25:43Z

Codecov Report

Merging #59 into master will decrease coverage by 2.1%.
The diff coverage is 65.85%.

@@            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

Impacted Files	Coverage Δ
src/tox_travis/toxenv.py	`89.87% <100%> (-0.49%)`	❌
src/tox_travis/after.py	`59.77% <50%> (+0.21%)`	✅
src/tox_travis/utils.py	`65.38% <57.14%> (-34.62%)`	❌
src/tox_travis/hooks.py	`78.57% <75%> (-1.43%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ccef42...14b1717. Read the comment docs.

jayvdb · 2017-06-12T22:12:58Z

src/tox_travis/hooks.py


-    override_ignore_outcome(config)
+    override_ignore_outcome(config, cfg)


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.

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.

codecov-io · 2017-10-22T05:10:43Z

Codecov Report

Merging #59 into master will increase coverage by 0.04%.
The diff coverage is 57.14%.

@@            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

Impacted Files	Coverage Δ
src/tox_travis/toxenv.py	`95.34% <100%> (-0.11%)`	⬇️
src/tox_travis/after.py	`57.83% <40%> (+0.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc25fa2...f4375ab. Read the comment docs.

ryanhiebert · 2017-10-28T03:05:45Z

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.

ryanhiebert · 2017-10-30T02:11:51Z

I'm going to clean this up and merge it.

Tests?

Yes, those too. I spoke like I wasn't going to, but I definitely do, and hopefully a green review too ;-)

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.

rpkilby

👍

ryanhiebert requested a review from rpkilby March 1, 2017 13:58

rpkilby reviewed Mar 2, 2017

View reviewed changes

rpkilby mentioned this pull request Mar 2, 2017

Set toxenvs in tox_configure instead of during parsing #61

Closed

ryanhiebert force-pushed the config-file branch from 2f3ef44 to 619e6ff Compare March 2, 2017 23:56

ryanhiebert force-pushed the config-file branch from 619e6ff to 14b1717 Compare March 10, 2017 06:17

jayvdb reviewed Jun 12, 2017

View reviewed changes

ryanhiebert force-pushed the config-file branch from 14b1717 to df2fed4 Compare October 22, 2017 05:10

ryanhiebert force-pushed the config-file branch from df2fed4 to 13ce1b2 Compare October 28, 2017 01:37

Use Tox parsed config

f4375ab

ryanhiebert force-pushed the config-file branch from 13ce1b2 to f4375ab Compare October 28, 2017 02:49

rpkilby approved these changes Oct 30, 2017

View reviewed changes

ryanhiebert merged commit ffe8441 into master Oct 31, 2017

ryanhiebert deleted the config-file branch October 31, 2017 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same config file as Tox #59

Use the same config file as Tox #59

ryanhiebert commented Mar 1, 2017

ryanhiebert commented Mar 1, 2017

rpkilby commented Mar 2, 2017

rpkilby left a comment

rpkilby Mar 1, 2017

ryanhiebert Mar 2, 2017

rpkilby Mar 2, 2017

ryanhiebert Mar 2, 2017

rpkilby Mar 2, 2017

ryanhiebert Mar 2, 2017

rpkilby Mar 2, 2017

ryanhiebert Mar 2, 2017

rpkilby Mar 2, 2017

rpkilby Mar 2, 2017

ryanhiebert Mar 2, 2017

rpkilby Mar 2, 2017

ryanhiebert commented Mar 2, 2017

rpkilby commented Mar 2, 2017 •

edited

Loading

ryanhiebert commented Mar 3, 2017

codecov bot commented Mar 10, 2017

jayvdb Jun 12, 2017

ryanhiebert Oct 28, 2017

codecov-io commented Oct 22, 2017 •

edited

Loading

ryanhiebert commented Oct 28, 2017

ryanhiebert commented Oct 30, 2017 •

edited

Loading

rpkilby left a comment


		override_ignore_outcome(config)
		override_ignore_outcome(config, cfg)

Use the same config file as Tox #59

Use the same config file as Tox #59

Conversation

ryanhiebert commented Mar 1, 2017

ryanhiebert commented Mar 1, 2017

rpkilby commented Mar 2, 2017

rpkilby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhiebert commented Mar 2, 2017

rpkilby commented Mar 2, 2017 • edited Loading

ryanhiebert commented Mar 3, 2017

codecov bot commented Mar 10, 2017

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 22, 2017 • edited Loading

Codecov Report

ryanhiebert commented Oct 28, 2017

ryanhiebert commented Oct 30, 2017 • edited Loading

rpkilby left a comment

Choose a reason for hiding this comment

rpkilby commented Mar 2, 2017 •

edited

Loading

codecov-io commented Oct 22, 2017 •

edited

Loading

ryanhiebert commented Oct 30, 2017 •

edited

Loading