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

ISort #24

Merged
merged 10 commits into from
Aug 18, 2020
Merged

ISort #24

merged 10 commits into from
Aug 18, 2020

Conversation

AlecRosenbaum
Copy link
Contributor

@AlecRosenbaum AlecRosenbaum commented Aug 18, 2020

Related: #12

This change:

  • add isort config
  • sorts imports using isort
  • enforces that import sorting within circleci
  • adds a devtools container to docker-compose.yml for running black and isort

@AlecRosenbaum AlecRosenbaum requested a review from thomasst August 18, 2020 13:15
Copy link
Member

@thomasst thomasst left a comment

Choose a reason for hiding this comment

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

I may have missed a few instances of the logger, but it should be below all the imports.

@@ -1,14 +1,16 @@
#!/usr/bin/env python
import click
Copy link
Member

Choose a reason for hiding this comment

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

Click should be moved down since monkey.patch_all() should be the first thing.


import click
import gevent_openssl
from setproctitle import setproctitle

gevent_openssl.monkey_patch()
Copy link
Member

Choose a reason for hiding this comment

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

While this script is probably not used at all, ideally the only import above this line would be gevent_openssl.

from nylas.logging import get_logger
from sqlalchemy.orm.exc import NoResultFound

log = get_logger()
Copy link
Member

Choose a reason for hiding this comment

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

Can probably move this below the imports?

from nylas.logging import get_logger
from sqlalchemy import Column, Integer, String

log = get_logger()
Copy link
Member

Choose a reason for hiding this comment

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

Move logger down?

)

from nylas.logging import get_logger

log = get_logger()
Copy link
Member

Choose a reason for hiding this comment

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

Move logger down?

from nylas.logging import get_logger
from nylas.logging.sentry import log_uncaught_errors
from sqlalchemy import desc

logger = get_logger()
Copy link
Member

Choose a reason for hiding this comment

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

Move logger down?

@AlecRosenbaum
Copy link
Contributor Author

Do we want to change functionality with this PR?

I had intentionally not moved any of the statements relative to import groups so as to not accidentally change any functionality impacted by these statements.

@AlecRosenbaum AlecRosenbaum marked this pull request as ready for review August 18, 2020 14:51
@thomasst
Copy link
Member

Do we want to change functionality with this PR?

I wouldn't consider this "changing functionality". We want to make sure imports are properly sorted, and I'd say this means we should move statements in between top-level imports to where they belong.

@AlecRosenbaum
Copy link
Contributor Author

Alright, I've moved the logging configuration everywhere I saw it out of place in this PR's diff.

@AlecRosenbaum AlecRosenbaum requested a review from thomasst August 18, 2020 17:21
Copy link
Member

@thomasst thomasst left a comment

Choose a reason for hiding this comment

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

just minor comment

from gevent import monkey

monkey.patch_all()
import gevent_openssl

gevent_openssl.monkey_patch()

import click
Copy link
Member

Choose a reason for hiding this comment

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

Could we have a newline above this import (and above import gevent_openssl)? This applies in a few other cases within this commit as well.

@AlecRosenbaum AlecRosenbaum merged commit 28b5adb into master Aug 18, 2020
@AlecRosenbaum AlecRosenbaum deleted the isort-files branch August 18, 2020 18:06
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