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

MNT: improve warnings #485

Merged
merged 6 commits into from Oct 28, 2021
Merged

Conversation

tacaswell
Copy link
Contributor

Came across this warning while working on another library. This PR:

  • removes the internal use of the deprecated module
  • adds the stacklevel argument to the warning so it shows the user where they are using it
  • fixes a future deprecation warning in the tests
  • makes pytest fail on warnings so they don't come back

@eriknw
Copy link
Member

eriknw commented Jul 19, 2020

Thanks, @tacaswell. This generally looks good to me.

There seems to be some difficulty running the new test while also running doctests, because choosing to run doctests imports toolz.compatibility. This seems like it'll be annoying to work around.

One option is to just remove the new test. Perhaps we could instead test this deprecation warning with a bash command in .travis.yml, such as:

python -c 'import toolz.compatibility' 2>&1 | grep "^<string>:1: DeprecationWarning: The toolz.compatibility module"

@tacaswell
Copy link
Contributor Author

We can also put that check in a subprocess call (we use that technique to test multiple GUI frameworks with Matplotlib).

@tacaswell
Copy link
Contributor Author

@eriknw any idea why CI is not firing?

@eriknw
Copy link
Member

eriknw commented Jul 1, 2021

Oh no! Probably because we haven't moved to Github Actions yet. I guess it's (past) time to do that. I can do it unless somebody else really, really wants to :)

Thanks for the ping.

@eriknw
Copy link
Member

eriknw commented Jul 1, 2021

Although, weird, did CI never fire for this PR?

@eriknw
Copy link
Member

eriknw commented Jul 1, 2021

Regardless, I plan to give toolz attention this weekend and will get this PR merged. Thanks @tacaswell

@tacaswell
Copy link
Contributor Author

It fired on the pre-rebase PR (which is what told us doctest was unhappy).

So it warns in the user's code not where it is defined.
toolz/tests/test_itertoolz.py::test_random_sample
toolz/tests/test_itertoolz.py::test_random_sample
toolz/tests/test_itertoolz.py::test_random_sample
toolz/tests/test_itertoolz.py::test_random_sample
  ../lib/python3.9/random.py:100: DeprecationWarning: Seeding based on hashing is deprecated
  since Python 3.9 and will be removed in a subsequent version. The only
  supported seed types are: None, int, float, str, bytes, and bytearray.
    self.seed(x)

explicitly hash the values before passing in
@tacaswell
Copy link
Contributor Author

@eriknw versioneer (?!?) needed some updating to install on py311 so I took the opportunity to rebase and fix this up again!

I gave up on making pytest warn on error because I could not sort out how to get doctest to not import the module that warns.

@eriknw
Copy link
Member

eriknw commented Oct 28, 2021

Thanks, you're the best @tacaswell! Yeah, I also had difficulty with it.

Merging!

@eriknw eriknw merged commit 55c60e9 into pytoolz:master Oct 28, 2021
@tacaswell tacaswell deleted the mnt_improve_warnings branch October 29, 2021 02:51
@tacaswell
Copy link
Contributor Author

Your welcome (and your too kind).

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.

None yet

2 participants