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

Revert "Less restrictive token characters indexer" #2497

Merged
merged 1 commit into from Feb 11, 2019

Conversation

Projects
None yet
3 participants
@matt-gardner
Copy link
Member

commented Feb 11, 2019

Reverts #2494

We just merged a huge involved PR to get rid of all of our warnings when we run tests (#2479, #2282). I won't repeat the justification for this here - anyone interested can go back and read that history. The bottom line: it is intentionally difficult to introduce new warnings into our code. The original PR that introduces a warning must also do whatever cleanup is necessary for it, so that the cleanup has a clear owner and actually gets done. This PR bypassed that by using a UserWarning instead of a DeprecationWarning, so I am reverting it. I had a discussion with @joelgrus, who hadn't followed all of the discussion involved in #2282, and approved the PR without realizing that we were intentionally disallowing this. Per @brendan-ai2's arguments, we are also intentionally slow at making API changes.

For @matt-peters, who made this change, there are three ways forward:

  1. (Easiest) Just copy the code for the class you want and make the change, registering it as something else. For a research project, this is generally the right thing to do. Once you have a clear, compelling use case, we can consider an API change to handle it.
  2. Make the change without adding a warning. I don't much like this solution, because it makes the API muddy without giving a warning about the muddiness.
  3. Use a DeprecationWarning instead of a UserWarning, and make all of the tests pass.

@matt-gardner matt-gardner requested a review from joelgrus Feb 11, 2019

@matt-gardner matt-gardner merged commit 56c11e5 into master Feb 11, 2019

3 checks passed

Pull Requests (AllenNLP Library) TeamCity build finished
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project Absolute coverage decreased by -<1% but relative coverage increased by +7% compared to 89056f1
Details

@matt-gardner matt-gardner deleted the revert-2494-mp/token_characters branch Feb 11, 2019

@matt-peters

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

I hadn't seen or followed the previous PRs, so was unaware of the restrictions against adding new warnings. I'll almost certainly take the easiest path here and hack a way around it.

@matt-gardner

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

No worries @matt-peters, I didn't think you meant to do anything bad. But yeah, the easiest path here is probably best from a library-stability perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.