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

Baseline Refactor #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ambsw-technology
Copy link

@ambsw-technology ambsw-technology commented Apr 29, 2019

Before submitting PRs to add new behaviors, I wanted to wrap my head around the current codebase. I started with some basic contributor-friendly changes:

  • add documentation
  • clarify variable names (e.g. local instead of l)
  • add test cases

I had to do a lot of mocking to get my first wave of test cases working. To reduce the complexity of test cases, I needed to rearrange code to better encapsulate behaviors. This PR is the product of that effort and includes changes like (edit: renamed State classes to clearly indicate storage backend):

  • To ensure consistent behavior through their lifecycle, all classes are now configured on __init__ (should avoid issues like Replace -p with ENV Variable #15 for programmatic users)
  • To support custom diffing classes/behaviors (e.g. issues Root Path Config #11, Ignore Encrypted Entries #13, and Improved concurrency handling in pull #16), DiffResolver (formerly FlatDictDiffer) is now injected into the ParameterStore (formely RemoteState) constructor.
    • To configure DiffResolver without coupling, I created a configure() classmethod that uses functools.partial()` to pre-populate constructor kwargs.
  • All flattening behavior is encapsulated in DiffResolver. This simplifies the interface on ParameterStore and YAMLFile (formely LocalState) which only accept nested dicts (though RemoteState uses some flattened endpoints).
  • Some of the changes (e.g. more git-like method names on DiffResolver) could be reverted, but I changed the signature of most/all methods so it wouldn't really help backwards compatibility.

This PR should be 100% backwards compatible for CLI users. Obviously, the interfaces for most classes/methods have changed so it will be a breaking change (i.e. major release) for programmatic users.

I know big rewrites aren't normally welcomed from a new contributor, but I hope you'll forgive the overhaul since it should simplify/enable additional improvements. I'm going to submit additional PRs, all based on this foundation. If you object to this direction, I can certainly try to rewrite more to your liking.

…, and better encapsulate behaviors (among other things to simplify testing)
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