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

Revamped module loading, addresses #202 issue #203

Merged
merged 1 commit into from Aug 27, 2015
Merged

Revamped module loading, addresses #202 issue #203

merged 1 commit into from Aug 27, 2015

Conversation

ghost
Copy link

@ghost ghost commented Aug 10, 2015

Revamped the module loading to be shorter and avoid certain bugs noted in #202

Old code features:

  • Always includes default module folder
  • Files inside default folder are subject to some validation (Don't start with "_" and end with ".py")
  • No validation if whitelisted modules exist in default folder
  • Direct file paths inside 'extra' are subject to no validation
  • Files insides folders, which are inside 'extra', are subject to no whitelisting/blacklisting logic
  • Files insides folders, which are inside 'extra', are subject to some validation (Don't start with "_" and end with ".py")
  • Only files inside default folder are subject to blacklisting/whitelisting

New code features:

  • Always includes default module folder
  • All files insides folders need to exist
  • All files insides folders are subject to some validation (Don't start with "_" and and end with ".py")
  • All files inside folders are subject to blacklisting/whitelisting
  • Direct file paths inside 'extra' are subject to no validation

@ghost
Copy link
Author

ghost commented Aug 10, 2015

Slightly relevant:
How does "overloading" modules sound? Basically instead of keeping an array of filenames:

filenames = [ "default/admin.py", "default/ask.py", "extra/admin.py" ]

We keep a dictionary<name, filename>, so that files that have the same name override each other instead of loading twice:

filenames = { "admin" => "extra/admin.py", "ask" => "default/ask.py" }

In the above example, since extra/admin.py was validated last it replaced default/admin.py.

@ghost
Copy link
Author

ghost commented Aug 10, 2015

I may have forgotten to rebase before pushing, hope that's not an issue, I was only 2 commits behind (the typo fix).

name = os.path.basename(fn)[:-3]
# If whitelist is present only include whitelisted
# Never include blacklisted items
if name in enabled or not enabled and name not in excluded:
Copy link
Contributor

Choose a reason for hiding this comment

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

You could make this clearer

Copy link
Author

Choose a reason for hiding this comment

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

I feel it's a lot more clearer than it was before.

The excluded and enabled variables aren't called "excluded_modules" and "enabled_modules" because it makes the lines really long, but if that's not a problem I can gladly rename them.

fn can probably be renamed to file, or filename.

Besides that I sadly can't think of a way to improve the code.

@kaneda
Copy link
Contributor

kaneda commented Aug 10, 2015

@Bekey one comment, do rebase against latest master and force push

@myano myano merged commit 0bd45e6 into myano:master Aug 27, 2015
@ghost ghost deleted the dev-fix-202-module-loading branch March 3, 2019 11:32
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