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

Added isolating states #26

Closed
wants to merge 2 commits into from
Closed

Added isolating states #26

wants to merge 2 commits into from

Conversation

jackd
Copy link

@jackd jackd commented Aug 20, 2019

I'm not sure if this is a direction you'd like to take this, and some would say encapsulation kind of defeats the purpose of the package - but occasionally I want to limit the effect of importing bindings, and there's only so much scopes can do.

There are a few changes that could be made - e.g. removing globals entirely and changing references to the members of the top stack state. Can't imagine the performance change would be great either way, but happy to be directed.

@dhr
Copy link
Contributor

dhr commented Aug 30, 2019

Hey, thanks, and very sorry for the slow response on this and the other pull requests. This CL is a great direction and something I've been meaning to do for some time. In particular I'd like to get to the point you mentioned where all of the currently global state is encapsulated in a single object (your GinState class), and the only remaining global points to an instance of that class.

Unfortunately our current GitHub syncing process is quite manual, and accepting pull requests is painful until we're done automating that a bit more. That's a high priority for us, but might take another two weeks or so. Once that's done I'd love to review this (and your other PRs) more thoroughly and get them merged in. Thanks for the contributions and sorry again for the slow turnaround!

@jackd
Copy link
Author

jackd commented Sep 2, 2019

I'm in no rush :). I've just done a rather large copy-paste job moving as seemd practical into the state object (renamed ConfigState because I realized it didn't quite capture everything to do with gin - e.g. it doesn't try to capture configurables themselves, just the bindings associated with them). It'll make the diff a bit gross, but that should just be cosmetic for any work done in the same config context block.

@dhr
Copy link
Contributor

dhr commented Jan 7, 2020

Hi, and sorry again for the long delays. We (finally) have our syncing mechanisms in place... so I'd like to get a version of this checked in. Whenever you have some time could you:

  1. Squash the two commits in this PR together (makes the sync/history easier to deal with...).
  2. Possibly move the tests from state_test into config_test (not sure if there was a reason to separate them.

I'll then do a more thorough review and import the changes.

Thanks for this PR! Looks great overall. And sorry again for the long delays.

@sguada
Copy link
Collaborator

sguada commented Jan 7, 2020

Could ConfigState be pickled?
Also would it make more sense to rename it to Config or Configuration?

@jackd
Copy link
Author

jackd commented Jan 17, 2020

Happy to make the proposed changes, but my PhD deadline is looming so I'll be pretty flat out for the next month. Looking to get onto it after that.

@dhr
Copy link
Contributor

dhr commented Jan 24, 2020

Sounds good! No rush. If I have a chance before then I can try importing and making the changes as well. Good luck finishing the PhD!

@jackd jackd mentioned this pull request Apr 23, 2020
@jackd
Copy link
Author

jackd commented Apr 23, 2020

@dhr Sorry for the delay getting back to this. Having learned first-hand the headaches of reviewing large diffs (and the fact that the conflict resolution would have been more work than just starting from scratch) I'm abandoning this in favour of #66.

@jackd jackd closed this Apr 23, 2020
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

3 participants