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

Enable flake8 code E501 #8680

Merged
merged 23 commits into from
Jun 3, 2021
Merged

Conversation

iurisevero
Copy link
Contributor

@iurisevero iurisevero commented May 12, 2021

Resolve #8490
Proposed changes:

  • Enable flake8 code E501 and fix all long lines alerts

Status (please check what you already did):

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

@sara-tagger sara-tagger requested a review from rctatman May 13, 2021 06:00
@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @rctatman will take a look at it as soon as possible ✨

@iurisevero iurisevero force-pushed the 8490-Enable-flake8-code-E501 branch from 4e9afa1 to 2c34e80 Compare May 13, 2021 18:12
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 - what a tremendous effort! Thanks for your help on this!

rasa/core/policies/ensemble.py Outdated Show resolved Hide resolved
rasa/core/tracker_store.py Outdated Show resolved Hide resolved
rasa/nlu/test.py Outdated Show resolved Hide resolved
rasa/shared/core/domain.py Outdated Show resolved Hide resolved
rasa/shared/importers/autoconfig.py Outdated Show resolved Hide resolved
@iurisevero
Copy link
Contributor Author

@wochinge I resolved all conversations, but every time I merge with main, it can break the CI in code quality. I'll fix it this last time and stop updating this branch, ok?

rasa/shared/core/domain.py Outdated Show resolved Hide resolved
rasa/shared/importers/autoconfig.py Outdated Show resolved Hide resolved
tests/nlu/featurizers/test_convert_featurizer.py Outdated Show resolved Hide resolved
@@ -75,16 +75,16 @@ async def test_create_train_data_no_history(domain: Domain, stories_path: Text):

assert hashed == [
Copy link
Contributor

Choose a reason for hiding this comment

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

the comments here and in test_telemetry are pretty extensive. What about ignoring the entire files for now?

Copy link
Contributor Author

@iurisevero iurisevero May 26, 2021

Choose a reason for hiding this comment

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

It's better to add a flake8: noqa or re-add the per-file-ignore at the setup?

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.

Amazing thanks! Can you please update the branch with latest main? I'll try to approve and merge asap then.

@iurisevero
Copy link
Contributor Author

LOL surprisingly there were no failed checks! @wochinge it's all yours xD

@wochinge wochinge merged commit d4d606b into RasaHQ:main Jun 3, 2021
@wochinge
Copy link
Contributor

wochinge commented Jun 3, 2021

Thanks for this change! @iurisevero Having this check automated will greatly help to maintain consistency in the codebase in the future 🙌🏻

@ErickGiffoni ErickGiffoni deleted the 8490-Enable-flake8-code-E501 branch August 5, 2021 19:41
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.

Enable flake8 code E501
3 participants