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

Feature/optimizations #89

Closed
wants to merge 4 commits into from

Conversation

ciupicri
Copy link

A couple of optimizations.

@wolph
Copy link
Owner

wolph commented Jun 16, 2021

I'm not sure I see the point of the optimisation to be honest. Beyond your personal style changes the code seems identical?

@ciupicri
Copy link
Author

A little bit of speed didn't hurt anyone and since the optimizations didn't require any radical changes, I implemented them. As you've mentioned, the code is practically the same, just a bit faster (~2% for my files and hardware & software setup).

@wolph
Copy link
Owner

wolph commented Jun 17, 2021

I'm honestly not a big fan of the from ... import ... style because it doesn't show where an import came from. In this case that is admittedly less of a problem, but it would be inconsistent with the rest of the source.

With regards to performance however, are you processing large enough files for that to be relevant? I have hardly looked at performance with this library because there hasn't been much need for it, but I'm sure I can greatly improve the performance with little effort if it is problematic that is.

@ciupicri
Copy link
Author

imaplib from the standard library has both import ... and from ... import, but I can't disagree with your preferences.

Parsing takes less than a minute for me, so it's bearable, but of course I wouldn't mind if it would take less.

@stale
Copy link

stale bot commented Nov 28, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 28, 2021
@wolph
Copy link
Owner

wolph commented Nov 28, 2021

imaplib from the standard library has both import ... and from ... import, but I can't disagree with your preferences.

Unfortunately the standard library isn't always the best example of pretty/Pythonic code. Take a look at the logging module for example, the naming and structure of that is very unlike Python.

I try to follow PEP 20 in that regard: https://www.python.org/dev/peps/pep-0020/
In this case specificially: "Explicit is better than implicit."

@stale stale bot removed the wontfix label Nov 28, 2021
@wolph wolph closed this May 14, 2022
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

2 participants