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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change registry mechanism to avoid circular imports #365

Merged
merged 1 commit into from Jun 7, 2019

Conversation

brunobord
Copy link
Member

@brunobord brunobord commented Jun 7, 2019

closes #288

  • As it's a refactor, tests are completely unchanged and mark no regression
  • Changelog amended with a mention describing your changes.

Rationale

In the ticket #288, it appeared that simply importing any given calendar would import all the library because of the "auto-registration" of calendars into the ISO registry.

You just need to run the following code and inspect the last lines of the output to witness it:

from trace import Trace
tracer = Trace()
# make run a simple import for a single country
tracer.run('from workalendar.europe.italy import Italy')
res = tracer.results()
# print results
res.write_results(coverdir='.cache', summary=True)

I've attached the trace-master.log here, which is an extract of the output when running against current master (v5.0.3).
trace-master.log

Improvements

This current PR has a very different output. As you can see in the trace-288.log which corresponds to the same code executed on this very branch.
trace-288.log

Here's an array showing the main differences:

master code branch "288-new-registry"
number of lines 229 139
load Africa modules yes 馃憥 no 馃啑
load America modules yes 馃憥 no 馃啑
load Asia modules yes 馃憥 no 馃啑
load Europe modules yes 馃啑 yes 馃啑
load Oceania modules yes 馃憥 no 馃啑
load Oceania modules yes 馃憥 no 馃啑
load USA modules yes 馃憥 no 馃啑

Important Note The API of the registry module hasn't changed, as all tests show no regression. However, it's expected to change, in order to return plain built-in dict objects instead of OrderedDict objects. A deprecation warning has been added to the Changelog and the code.

@brunobord
Copy link
Member Author

pinging @sevdog, since it was their issue at first.

@brunobord
Copy link
Member Author

aha! I'm taking this 馃憤 as a "yes, that's cool, you can squash and merge!" (too bad github doesn't send a small notification on emoji reactions. Something like a cool sound when you receive one)

@brunobord brunobord merged commit 1b1440f into master Jun 7, 2019
@brunobord brunobord deleted the 288-new-registry branch June 7, 2019 13:44
@sevdog
Copy link
Contributor

sevdog commented Jun 10, 2019

@brunobord Yes, good 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.

Avoid circular import in registry
2 participants