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

#1021 - refactoring: delete unused imports #536

Merged

Conversation

blankoworld
Copy link
Contributor

More and more imports are useless in the project.

To avoid this problem, 2 things are needed:

  • delete current imports
  • check imports regularly

To delete current imports it uses a program: autoflakes

autoflake --remove-all-unused-imports -i -r --exclude ui \
--ignore-init-module-imports .

Also checks that no more useless imports remains. It also uses
autoflakes during run-tests.sh script.

Copy link

@BadrAly BadrAly left a comment

Choose a reason for hiding this comment

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

I do not know if we are allowed to have execution commands on the commit message. I will leave this to @iGormilhit to confirm.

Copy link
Contributor

@rerowep rerowep left a comment

Choose a reason for hiding this comment

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

Are the celery worker tasks: indexer, accounts, ebooks-harvester, notification-creation still functional and executed in a celery worker?

@blankoworld
Copy link
Contributor Author

Are the celery worker tasks: indexer, accounts, ebooks-harvester, notification-creation still functional and executed in a celery worker?

That's a good point. Have I any way to check this quickly @rerowep ?

@rerowep
Copy link
Contributor

rerowep commented Sep 30, 2019

Are the celery worker tasks: indexer, accounts, ebooks-harvester, notification-creation still functional and executed in a celery worker?

That's a good point. Have I any way to check this quickly @rerowep ?

  • bootstrap
  • start server with celery workers pipenv run celery worker -A invenio_app.celery --autoscale=8,1 --beat -l WARNING --without-heartbeat & pid_celery=$!
  • setup
  • control if you see errors in the server log after some time

@blankoworld blankoworld force-pushed the doo-#1021-remove-python-unused-imports branch 2 times, most recently from 6802136 to 9b2f489 Compare October 1, 2019 09:31
Copy link

@iGormilhit iGormilhit left a comment

Choose a reason for hiding this comment

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

About the commit message:

* Uses autoflake to remove current useless python imports. The command is `autoflake --remove-all-unused-imports -i -r --exclude ui --ignore-init-module-imports`.
* Uses autoflake in `run-tests.sh` script, to check for useless python imports regularly.

I do like informative commit message, but let's try to not be too verbose.

@rerowep rerowep self-requested a review October 1, 2019 14:03
Copy link
Contributor

@rerowep rerowep left a comment

Choose a reason for hiding this comment

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

Are the celery worker tasks: indexer, accounts, ebooks-harvester, notification-creation still functional and executed in a celery worker?

@blankoworld blankoworld force-pushed the doo-#1021-remove-python-unused-imports branch 8 times, most recently from 06b2a87 to 1a31dd2 Compare October 8, 2019 14:05
More and more imports are useless in the project.

To avoid this problem, 2 things are needed:

* delete current imports
* check imports regularly

To delete current imports it uses a program: `autoflakes`.

Also checks that no more useless imports remains. It also uses
`autoflakes` during `run-tests.sh` script.

Co-Authored-by: Olivier DOSSMANN <[email protected]>
@blankoworld blankoworld force-pushed the doo-#1021-remove-python-unused-imports branch from 1a31dd2 to 85fc950 Compare October 8, 2019 14:38
@iGormilhit iGormilhit merged commit 5d4806c into rero:dev Oct 9, 2019
@blankoworld blankoworld deleted the doo-#1021-remove-python-unused-imports branch October 9, 2019 14:25
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.

5 participants