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

Code quality improvements #273

Merged
merged 12 commits into from Oct 24, 2016
Merged

Conversation

wojcikstefan
Copy link
Member

This is a follow-up after #270. I tightened flake8's code checking, added flake8-import-order and fixed existing imports (see https://www.python.org/dev/peps/pep-0008/#imports for details), and added some minor style tweaks.

@lafrech let me know what you think :)

@lafrech
Copy link
Member

lafrech commented Oct 23, 2016

Looks good! Great!

Did you intentionally remove host uri parsing in _resolve_settings?

@wojcikstefan
Copy link
Member Author

Did you intentionally remove host uri parsing in _resolve_settings?

Yep, I removed it since it was commented out. If we need to bring it back, it's still in the git history. Appreciate you double-checking it though!

@wojcikstefan
Copy link
Member Author

I just did a self-review as a sanity check and everything looks good to me. One other thing I'd like to get rid of completely is the ugly dict.items() if (sys.version_info >= (3, 0)) else dict.iteritems() style. It makes the code less readable and the performance boost we'd get in Py2.x from using iteritems is negligible. We're talking about microseconds in a package that by nature interacts with databases (think milliseconds at least). Readability is more important is such cases.

@lafrech
Copy link
Member

lafrech commented Oct 24, 2016

Oh, I didn't see that code was commented out (it wasn't obvious when using colored split diff on GitHub).

TBH, I didn't check every line. I trust you on keeping this low-level and sticking to style changes. Hence my surprise about what appeared to me like a feature removal.

I also hope the tests would have pointed a breaking change.

I'm merging this.

Thanks.

BTW, you're achieving a lot of work on flask-mongoengine, lately. Would you like to join the team?

@lafrech lafrech merged commit 3777f74 into MongoEngine:master Oct 24, 2016
@wojcikstefan
Copy link
Member Author

I trust you on keeping this low-level and sticking to style changes.

Thanks. I like sending separate PRs for separate issues so that things are easier to review, easier to revert, and easier to blame :) Hence this PR didn't alter any logic.

BTW, you're achieving a lot of work on flask-mongoengine, lately. Would you like to join the team?

I'd love to! We just started working on a new service at my company and we decided to use upstream mongoengine/flask-mongoengine (as opposed to our forks), hence I expect to pay a lot of attention to this repo.

@lafrech
Copy link
Member

lafrech commented Oct 24, 2016

@MongoEngine/flask-mongoengine, @MongoEngine/mongoengine, could anybody with admin privileges add @wojcikstefan to the organization and to both teams?

(And perhaps also give me admin privileges on flask-mongoengine repo. Apparently, I can only access MongoEngine settings.)

Thanks.

@wojcikstefan
Copy link
Member Author

@MongoEngine/flask-mongoengine, @MongoEngine/mongoengine looking forward to being part of the team and contributing more :)

@wojcikstefan
Copy link
Member Author

@lafrech do you have a way to contact the admins? I'd love to join the team sooner rather than later, so that I can spend some spare time triaging issues :)

@lafrech
Copy link
Member

lafrech commented Oct 27, 2016

@thedrow
Copy link

thedrow commented Oct 27, 2016

My apologies for the late response.
@wojcikstefan I just added you to the flask-mongoengine team.
@lafrech I made you the maintainer of the team.
Great job!

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