Skip to content

Fixes #42643 allow keys with colons in JSON config#66886

Closed
SteveDunn wants to merge 17 commits intodotnet:mainfrom
SteveDunn:42643-bind-to-json-containing-keys-with-colons
Closed

Fixes #42643 allow keys with colons in JSON config#66886
SteveDunn wants to merge 17 commits intodotnet:mainfrom
SteveDunn:42643-bind-to-json-containing-keys-with-colons

Conversation

@SteveDunn
Copy link
Contributor

@SteveDunn SteveDunn commented Mar 19, 2022 •
edited
Loading

Each provider has their own Separator field (defaults to :). If users have colons in their keys, they can provide ` (or similar) as the optional parameter to the various methods.

Fixes #42643

@SteveDunn
42643 Work in Progress attempt at getting this to work. Note - hard-c…
03312e5 
…oded jsonfileprovider to backtick which'll probably break lots of tests
@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-Extensions-Configuration and removed community-contribution Indicates that the PR has been added by a community member labels Mar 19, 2022
@ghost
Copy link

ghost commented Mar 19, 2022

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Note: hardcoded, for now, jsonfileprovider to delimit by backtick which'll probably break lots of tests

Author: SteveDunn
Assignees: -
Labels:

area-Extensions-Configuration
Milestone: -

@SteveDunn SteveDunn changed the title 42643 Work in Progress attempt at getting this to work #42643 Work in Progress attempt at getting this to work Mar 19, 2022
@SteveDunn SteveDunn changed the title #42643 Work in Progress attempt at getting this to work Fixes 42643 Work in Progress attempt at getting this to work - https://github.com/dotnet/runtime/issues/42643 Mar 19, 2022
@SteveDunn SteveDunn mentioned this pull request Mar 19, 2022
Closed
@SteveDunn
Can now have colons in keys in json, but it still breaks other tests.…
712b165 
… Need the provider to specify their delimiter (to do next)
@SteveDunn
Got backtick separator working for json provider.
cd82ca5 
Now need to remove previous effort of passing separators everywhere
This was referenced Mar 21, 2022
Closed
Closed
@SteveDunn SteveDunn changed the title Fixes 42643 Work in Progress attempt at getting this to work - https://github.com/dotnet/runtime/issues/42643 Fixes 42643 allow keys with colons in JSON - https://github.com/dotnet/runtime/issues/42643 Mar 22, 2022
@SteveDunn SteveDunn changed the title Fixes 42643 allow keys with colons in JSON - https://github.com/dotnet/runtime/issues/42643 Fixes #42643 allow keys with colons in JSON Mar 22, 2022
@SteveDunn SteveDunn changed the title Fixes #42643 allow keys with colons in JSON Fixes #42643 allow keys with colons in JSON config Mar 22, 2022
@tebeco
Copy link

tebeco commented Mar 23, 2022

see #42643 (comment) for using array instead of direct keys

you might get more flexible code for the original need

maryamariyan
maryamariyan suggested changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@maryamariyan maryamariyan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is this PR still work in progress? I switched it to a draft for now.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 25, 2022
@maryamariyan maryamariyan marked this pull request as draft March 25, 2022 20:51
@SteveDunn
Copy link
Contributor Author

SteveDunn commented Mar 25, 2022

Is this PR still work in progress? I switched it to a draft for now.

I moved it from WIP as all it fixes the problem reported in the Issue. As you can see though, it did require some overloads to allow the user to specify their new separator string for the keys. I couldn't think of another way of allowing this, so I'm open to suggestions and ideas.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 25, 2022
@SteveDunn SteveDunn marked this pull request as ready for review April 1, 2022 05:49
@SteveDunn
Copy link
Contributor Author

SteveDunn commented Apr 1, 2022

Is this PR still work in progress? I switched it to a draft for now.

I moved it from WIP as all it fixes the problem reported in the Issue. As you can see though, it did require some overloads to allow the user to specify their new separator string for the keys. I couldn't think of another way of allowing this, so I'm open to suggestions and ideas.

... and, 6 days later I find the button that really does move it from draft! 🤦

@SteveDunn SteveDunn mentioned this pull request Apr 5, 2022
Open
@SteveDunn
Copy link
Contributor Author

SteveDunn commented Apr 5, 2022

Closed for now until the API Proposal is reviewed:
#67616

@SteveDunn SteveDunn closed this Apr 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@maryamariyan maryamariyan maryamariyan requested changes

@AraHaan AraHaan AraHaan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Extensions-Configuration

Projects

None yet

Milestone

No milestone

4 participants

@SteveDunn @tebeco @maryamariyan @AraHaan