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

Change warnings to python standard #4673

Merged
merged 34 commits into from Nov 16, 2019
Merged

Change warnings to python standard #4673

merged 34 commits into from Nov 16, 2019

Conversation

imLew
Copy link
Contributor

@imLew imLew commented Oct 25, 2019

Proposed changes:

  • Make rasa codebase conform to the python convention that warnings.warn should be used for warnings that the user needs to address while logger.warning should be used for warnings that the user should be aware of but cannot change themselves.
  • Changed strings that I encountered during these changesto f-strings in lieu of using "".format

See #4356
The next step will be to apply pytest.mark.filterwarnings to all those tests raising warnings.warn; but this should be merged first

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog
  • reformat files using black (please check Readme for instructions)

Nikolai and others added 4 commits October 7, 2019 17:48
replaced many `logger.warnings` with `warnings.warn`,
replaced some `.format` with f-strings
@imLew imLew requested a review from wochinge October 25, 2019 15:19
@imLew
Copy link
Contributor Author

imLew commented Oct 25, 2019

@wochinge this is a WIP because it should only be merged together with additional changes, however it definitely does need review at this stage.

@imLew imLew changed the title Warnings in tests Change warnings to python standard Nov 1, 2019
@imLew imLew marked this pull request as ready for review November 1, 2019 09:23
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Wow! Great work in applying these best practices to our code base!

We can also do that incrementally. So merge this and then do another PR for the rest?

rasa/core/channels/facebook.py Show resolved Hide resolved
rasa/core/channels/slack.py Outdated Show resolved Hide resolved
rasa/core/events/__init__.py Outdated Show resolved Hide resolved
rasa/core/featurizers.py Outdated Show resolved Hide resolved
rasa/core/policies/mapping_policy.py Outdated Show resolved Hide resolved
rasa/nlu/training_data/training_data.py Outdated Show resolved Hide resolved
rasa/nlu/training_data/training_data.py Outdated Show resolved Hide resolved
rasa/nlu/training_data/util.py Outdated Show resolved Hide resolved
rasa/server.py Outdated Show resolved Hide resolved
rasa/cli/utils.py Show resolved Hide resolved
Nikolai and others added 3 commits November 1, 2019 13:29
Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>
typos and f-strings messed up by black

Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>
Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>
@imLew
Copy link
Contributor Author

imLew commented Nov 1, 2019

We can also do that incrementally. So merge this and then do another PR for the rest?

Actually this should be everything (up to the date I created this PR), I looked at every instance of logger.warning and warnings.warn in rasa for this, so everything that's not somehow marked here should be correct usage (+-)

@wochinge
Copy link
Contributor

wochinge commented Nov 4, 2019

Awesome, I think there is one more open comment, but otherwise we are good 👍

@tmbo
Copy link
Member

tmbo commented Nov 13, 2019

@imLew can you please update this PR with latest master and please let Tobias know when this is ready for another review

@imLew imLew requested a review from wochinge November 14, 2019 09:51
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Left a couple of suggestions and two warnings correction, but ready to be merged for me 🚀 The warnings assertion in the tests look cool 💯

rasa/core/agent.py Outdated Show resolved Hide resolved
rasa/core/agent.py Outdated Show resolved Hide resolved
rasa/core/domain.py Outdated Show resolved Hide resolved
rasa/core/interpreter.py Outdated Show resolved Hide resolved
rasa/core/interpreter.py Show resolved Hide resolved
rasa/core/validator.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/embedding_intent_classifier.py Outdated Show resolved Hide resolved
rasa/utils/common.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
Nikolai and others added 3 commits November 14, 2019 11:51
Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>
Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>
Co-Authored-By: Tobias Wochinger <t.wochinger@rasa.com>
@imLew imLew merged commit 57c3632 into master Nov 16, 2019
@imLew imLew deleted the warnings-in-tests branch November 16, 2019 17:05
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

3 participants