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

Allow slash-delimited paths #21

Closed
colinodell opened this issue Sep 19, 2020 · 4 comments
Closed

Allow slash-delimited paths #21

colinodell opened this issue Sep 19, 2020 · 4 comments

Comments

@colinodell
Copy link
Contributor

colinodell commented Sep 19, 2020

I'd like to use this library in the next major release of league/commonmark. Unfortunately I'm blocked by the fact that v1 of my library uses array paths with slashes (foo/bar) instead of dots (foo.bar) and I'd like to preserve that for backward-compatibility reasons.

I'd therefore like to propose the ability for the library to allow for both . and / as key path delimiters.

I have working code I'd be happy to contribute, but it would rely on #18 (merged) and #20 to be merged first to avoid merge conflicts.

@simensen
Copy link
Member

Is it a configuration / opt-in thing to support both . and /, or would it be an "always allow both" sort of situation?

@colinodell
Copy link
Contributor Author

I'd strongly prefer this to be "always allow both" so that anyone receiving a DataInterface instance knows they can safely use either delimiter; otherwise, they might try to use / not knowing that it wasn't configured/enabled that way and get unexpected results.

I'd also be open to an alternative approach that allows Data to be extended and the delimiters to be set in that new class. Perhaps Data could have a static protected property or method like $delimiters or getDelimiters() that I could override. This has a similar effect where anyone using my subclass can safely assume that slashes are available.

One other potential approach could be setting the allowed delimiters in the constructor as an optional second parameter. This doesn't alleviate my primary concern but it would be a workable solution I could live with :)

I would not be in favor of something that configures this behavior statically as I'd like some level of guarantee that other code can't modify the settings globally in a way that could prevent / from being used.

Open to your thoughts here!

@colinodell
Copy link
Contributor Author

@simensen I have implemented both possible approaches for your review:

I'd appreciate any feedback you might have. And if/when you choose to merge one, please make sure to only merge one and close the other 😉

@simensen
Copy link
Member

I am closing this as I think the merging of #24 resolves this issue.

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

No branches or pull requests

2 participants