Skip to content

Conversation

@maryokhin
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the '.' removal intentional?
I don't particularly mind either ways, but we're using eg django1.7, so seems we may as well stick with the same python style (also it'd keep the changeset minimal, which'd be good for reviewing this)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, I see your comment above now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's intentional. I think the dot syntax looks much better, but because tox provides (see the note) the environments, it also knows how to work with basepython by looking at the prefix. With a custom prefix we would either have to redefine all the default envs (which doesn't make that much sense just for prettying stuff up) or override all the env created from the multi-dimensional matrix, which would make the whole refactoring pretty much useless. This is an example of how Tornado redefined theirs https://github.com/tornadoweb/tornado/blob/master/tox.ini#L52.

@lovelydinosaur
Copy link
Contributor

Looks great after the change!

The test failures were my fault (template change that I pushed directly to master, didn't expect it to cause failing tests)
I've now restarted the travis test run.

lovelydinosaur added a commit that referenced this pull request Nov 25, 2014
Remove boilerplate code from tox config
@lovelydinosaur lovelydinosaur merged commit 85f5e79 into encode:master Nov 25, 2014
@lovelydinosaur
Copy link
Contributor

Noting for other's reference that I needed to do rm -r .tox after this change, in order to resolve the following.

tox.ConfigError: ConfigError: substitution key 'py26,py27' not found

Not a problem but figured I'd post that somewhere visible so easier to dig out again if anyone is searching with same problem.

@jpadilla
Copy link
Contributor

This is awesome, updating the cookiecutter to include these changes.

@lovelydinosaur
Copy link
Contributor

updating the cookiecutter to include these changes.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants