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

fix flake8 config and drop unused code #8428

Merged
merged 31 commits into from
Apr 14, 2021
Merged

fix flake8 config and drop unused code #8428

merged 31 commits into from
Apr 14, 2021

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Apr 9, 2021

Proposed changes:

  • drop unused code
  • re-configure flake8 to flag unused code / imports

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)

@wochinge wochinge requested a review from m-vdb April 9, 2021 18:47
@wochinge wochinge marked this pull request as ready for review April 9, 2021 19:30
Copy link
Collaborator

@m-vdb m-vdb left a comment

Choose a reason for hiding this comment

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

Do you have an example of how it runs in the CI? I can't find an example of workflow with a failing check. Do we need some sort of documentation to explain what to do if the check fails?

rasa/core/channels/twilio.py Outdated Show resolved Hide resolved
rasa/core/featurizers/single_state_featurizer.py Outdated Show resolved Hide resolved
rasa/nlu/utils/__init__.py Show resolved Hide resolved
rasa/shared/core/events.py Show resolved Hide resolved
rasa/shared/data.py Show resolved Hide resolved
rasa/shared/importers/autoconfig.py Show resolved Hide resolved
rasa/shared/nlu/training_data/message.py Show resolved Hide resolved
tests/nlu/test_persistor.py Show resolved Hide resolved
@wochinge
Copy link
Contributor Author

pushed a commit which triggers a vulture error. Also happy to skip adding vulture to the CI for now. It's not perfect but in my opinion a good start to have a least some check for it.

@m-vdb
Copy link
Collaborator

m-vdb commented Apr 12, 2021

@wochinge it didn't fail because the lint for docstrings failed first :D

I don't think it's a good idea to not add it to the CI, because it will become dead code too (a bit ironic ahah)

@wochinge wochinge changed the title Add Vulture to CI and drop unused code fix flake8 config and drop unused code Apr 12, 2021
ignore = W503, # line break before binary operator
E203, # whitespace before ‘:’
E231, # missing whitespace after ‘,’, ‘;’, or ‘:’
E501, # too long lines - FIX THIS IN SEPARATE PR
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will tackle this separately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wochinge wochinge requested a review from m-vdb April 12, 2021 13:01
@wochinge
Copy link
Contributor Author

wochinge commented Apr 12, 2021

I dropped vulture and now use flake8 instead @m-vdb Removed more unused imports - flake8 is a lot better than vulture for this. Dropped vulture since it has too many false positives.

@m-vdb
Copy link
Collaborator

m-vdb commented Apr 12, 2021

but can flake8 detect dead code? unused import are a subset of dead code

@wochinge
Copy link
Contributor Author

wochinge commented Apr 12, 2021

not as good as vulture - but vulture has too many false positives so we can't really run it as part of the CI

@wochinge
Copy link
Contributor Author

@m-vdb Would be great if we could get this in before my vacation 👀 The review should be quick despite its size as it's just removing dead code.

rasa/cli/export.py Show resolved Hide resolved
@@ -6,7 +6,7 @@
from rasa.cli import SubParsersAction
from rasa.cli.arguments import run as arguments
import rasa.cli.utils
import rasa.shared.utils.cli
import rasa.shared.utils.cli # noqa: F401
Copy link
Collaborator

Choose a reason for hiding this comment

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

weird because it's used 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know - seems to be a false positive. Same as in `visualize - haven't found a way around it unfortunately.

rasa/shared/data.py Show resolved Hide resolved
rasa/shared/nlu/training_data/message.py Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
rasa/nlu/components.py Show resolved Hide resolved
Copy link
Collaborator

@m-vdb m-vdb left a comment

Choose a reason for hiding this comment

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

⭐ thanks for the quick turnaround!

@wochinge wochinge enabled auto-merge April 14, 2021 16:33
@wochinge wochinge merged commit a0d9cd0 into main Apr 14, 2021
@wochinge wochinge deleted the misc/vulture branch April 14, 2021 16:48
@wochinge wochinge self-assigned this Apr 19, 2021
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.

2 participants