Pull requests

#74 Declined
Repository
cjerdonek/tox-125-randomseed tox-125-randomseed
Branch
default
Repository
hpk42/tox tox
Branch
default

Implement --hashseed option for issue #125 (tests still needed).

Author
  1. Chris Jerdonek
Reviewers
Description

This is a new pull request for issue #125 (random hash seed) that incorporates the comments from pull request #73. The pull request is for discussion purposes to get feedback on the proposed API, rather than a pull request for final consideration. The proposed documentation and behavior is complete, but no tests were included. Tests can be written after there is agreement on the API.

By the way, I'll be out of town for several days so probably won't be able to update the patch for at least a week. I will probably be able to respond to comments though. Thanks!

Comments (5)

  1. Chris Jerdonek author

    By the way, I took away having a substitution string {hashseed} because I didn't think it's necessary anymore now that Tox sets PYTHONHASHSEED for you. Also, other programs (e.g. any Python test scripts being called) can access the value by looking at the environment variable itself, so there would be no need for example to pass {hashseed} on the command-line explicitly via a command in tox.ini.

  2. holger krekel repo owner

    I like the doc and the attempt at minimizing changes, makes sense! However:

    • I'd like to avoid introducing two options in an effort to keep number of optins down. Could you rather make it so that if --hashseed "" (i.e. an empty string) is used this means to not touch HASHSEED (a test environment setting it would still set it i guess)?

    • the implementation currently sets hashseed globally, i.e. does not really implement what the docs say.

    With this fixed, i'd be happy to get the final PR with tests and merge it :)

  3. Chris Jerdonek author

    Thanks for the comments! Hmm, yes, I also thought that having two options isn't ideal. But I didn't like the idea of a "magic" value either that causes PYTHONHASHSEED not to be touched. For example, what if the user wants to set PYTHONHASHSEED to the empty string from the command-line? How about the idea of making the argument to --hashseed optional (i.e. nargs='?'), in which case --hasheed would cause it not to be set while --hashseed "" would cause it to be set to the empty string?

    (Incidentally, does Tox let you unset an environment variable from tox.ini, as opposed to setting it to the empty string?)

    Regarding setting the hashseed globally, I'm not quite sure I understand the distinction. In the docs, I was intending for hashseed to be a single global value used for all environments (but that can be overridden in a testenv if necessary). (By "once per test run," I meant once per invocation of tox rather than once per test env.) Are you just saying you don't want the value to be stored on the global config object as config.hashseed? Or are you referring to the fact that PYTHONHASHSEED is being set before all calls to popen, as opposed to only prior to running test commands (i.e. only in the test method)? I agree that this latter issue should be fixed. I only noticed it after. Thanks for clarifying!

  4. holger krekel repo owner

    i don't think optional arguments to an option can work. They only work for positional arguments i'd think. Using a non-number or an empty string as a special value for specifying "clear PYTHONHASHSEED" is fine i think. --hashseed=unset for example. After all hashseeds otherwise need to be numbers.

    I am not exactly on the implementation, i.e. where to store the hashseed value. Maybe config.hashseed is fine. But i'd first try to have it populate the setenv setting (if it doesn't contain a PYTHONHASHSEED setting already) and that's it. This could be done wholly at configuration time and would be effective during install and test commands (IIRC that is how setenv is applied).

  5. Chris Jerdonek author

    Sounds good re: setenv, I'll update the PR. I'll also show you what I had in mind for --hashseed. Optional args for options are supported (e.g. as in the Python docs I linked to above).