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

Lazy sources - after submodules #96

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

beasteers
Copy link
Contributor

@beasteers beasteers commented Jun 8, 2020

I know I keep doing new PRs, sorry. Hopefully this will be the last one!!

Basically the goal is: ConfigSources manage their entire state and minimal assumptions are made about the internal workings of a ConfigSource by other Config objects (making ConfigSources much more extendable).

So for example, say you have some custom complicated source that is made by combining multiple files and an API call (idk why). By abstracting away ConfigSource.exists for example, the config source can define that state on its own without Configuration ever having to be privy to those details.

Notes:

  • we still have that bug where python 2.7 doesn't actually call dict.keys (or anything else) when casting to dict, so a hotfix is to look for keys in __getattribute__ and call load if keys was accessed. Obviously, I'd like to get rid of this, but I'm not sure what the best way is.
  • this is built on the submodule breakup PR so once that is merged, we should only have commits starting at 2ea14db
  • pypy is currently failing (like the 2.7 dict bug) because I'm guessing that it doesn't get keys when casting to dict. I'm guessing that because ConfigSource subclasses dict, it just does isinstance(src, dict)

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Merged with master for legibility! Let me know if I broke anything obvious.

Because this is a fairly sensitive change and I would love to avoid breaking existing code, I'm thinking of trying to do this in a few discrete steps that are even more granular. These steps come to mind:

  • The ConfigSource.of changes that in turn make Config.add and Config.set much simpler. This touches several pieces at once but should be fairly clearly behavior-preserving.
  • Add YamlSource.
  • Finally, switch LazyConfig to use all of that stuff instead of the current logic for lazy loading. This is the most potentially-accidentally-breaking change so we'll want to test it with real dependent projects (e.g., beets) somewhat thoroughly.

Is that phasing too outlandish?


def _load_first(func):
'''Call self.load() before the function is called - used for lazy source
loading'''
Copy link
Member

Choose a reason for hiding this comment

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

Let's switch this to """ for uniformity.

"""
def __init__(self, value, filename=None, default=False):
super(ConfigSource, self).__init__(value)
'''
Copy link
Member

Choose a reason for hiding this comment

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

Likewise with the docstring """s.

and os.path.splitext(value)[1] in YamlSource.EXTENSIONS)

def _load(self):
'''Load the file if it exists.'''
Copy link
Member

Choose a reason for hiding this comment

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

One more.

@beasteers
Copy link
Contributor Author

I think we could do 1+2, and then do 3 separately. 1 doesn't do much for simplification unless we have 2 in place as well.

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