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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 17 commits into from
Closed

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

wants to merge 17 commits into from

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 the community-contribution Indicates that the PR has been added by a community member label Mar 19, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added 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 requested changes Mar 25, 2022
View reviewed changes
Copy link
Member

@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.

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved
...aries/Microsoft.Extensions.Configuration.Json/ref/Microsoft.Extensions.Configuration.Json.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Json/tests/JsonConfigurationTest.cs Outdated Show resolved Hide resolved
...libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs Outdated Show resolved Hide resolved
...Extensions.Configuration.Abstractions/ref/Microsoft.Extensions.Configuration.Abstractions.cs Show resolved Hide resolved
@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

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

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

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

@maryamariyan maryamariyan maryamariyan requested changes

@AraHaan AraHaan AraHaan left review comments

Assignees
No one assigned
Labels
area-Extensions-Configuration
Projects
None yet
Milestone
No milestone
4 participants
@SteveDunn @tebeco @maryamariyan @AraHaan