Skip to content

System.Text.Json file and converter name clean-up (no functional changes) #32184

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
Merged
steveharter merged 3 commits into from
Feb 13, 2020
Merged

System.Text.Json file and converter name clean-up (no functional changes) #32184

steveharter merged 3 commits into from
Feb 13, 2020

Conversation

@steveharter
Copy link
Contributor

@steveharter steveharter commented Feb 12, 2020

General clean-up of converters and the System.Text.Json.csproj.

S.T.Json.csproj:

  • Use default sorting in Visual Studio. After this, as long as we use VS to add files etc then we shouldn't have to manually edit the .csproj for alphabetical sorting.

Converters (internal converters only)

  • Move converters to sub folders (Collection, Value and Object). There are too many in the current folder (52). Keep the same namespace.
  • Remove "Json" prefix for converter class name.
  • Ensure "Factory" is appended to each converter factory (not consistent today).
  • Ensure every converter's file name == class name.
  • To preserve git history, use separate commits with merge commit so git treats these as renames not delete+add.

Misc

  • Delete unused file (JsonPreservableArrayReference.cs)
  • Move\rename ExtensionMethods.cs to Converters\Collection\IEnumerableConverterFactoryHelpers.cs

@steveharter steveharter added NO-SQUASH The PR should not be squashed area-System.Text.Json labels Feb 12, 2020
@steveharter steveharter added this to the 5.0 milestone Feb 12, 2020
@steveharter steveharter self-assigned this Feb 12, 2020
@steveharter steveharter force-pushed the FileCleanup branch 2 times, most recently from 04621bd to 208e067 Compare February 12, 2020 20:33
jozkee
jozkee approved these changes Feb 12, 2020
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Otherwise; looks good to me.

...Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs Outdated
@@ -9,7 +9,8 @@ namespace System.Text.Json.Serialization.Converters
/// <summary>
/// Default base class implementation of <cref>JsonIEnumerableConverter{TCollection, TElement}</cref>.
/// </summary>
internal abstract class JsonIEnumerableDefaultConverter<TCollection, TElement> : JsonCollectionConverter<TCollection, TElement>
internal abstract class IEnumerableDefaultConverter<TCollection, TElement>
: JsonCollectionConverter<TCollection, TElement>
Copy link
Member

@jozkee jozkee Feb 12, 2020

Choose a reason for hiding this comment

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

JsonConllectionConverter, JsonDictionaryConverter and JsonObjectConverter are not renamed becuase those are the ones that are going to become public, right?

Copy link
Contributor Author

@steveharter steveharter Feb 13, 2020

Choose a reason for hiding this comment

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

Correct. If that's not the case then we can revisit the naming at that point.

ahsonkhan
ahsonkhan reviewed Feb 12, 2020
View reviewed changes
ahsonkhan
ahsonkhan approved these changes Feb 12, 2020
View reviewed changes
Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Other than that, looks good.

This was referenced Feb 13, 2020
Closed
Closed
Closed
Closed
@steveharter
Copy link
Contributor Author

steveharter commented Feb 13, 2020

Test failures unrelated (issues logged):

  • System.Security.Cryptography.Pkcs.Tests
  • System.Runtime.Loader.DefaultContext.Tests
  • System.Utf8String.Experimental.Tests
  • System.Net.Security.Tests

@steveharter steveharter merged commit 0e0e852 into dotnet:master Feb 13, 2020
@steveharter steveharter deleted the FileCleanup branch February 13, 2020 21:50
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jozkee jozkee jozkee approved these changes

@layomia layomia Awaiting requested review from layomia

+1 more reviewer

@ahsonkhan ahsonkhan ahsonkhan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@steveharter steveharter

Labels

area-System.Text.Json NO-SQUASH The PR should not be squashed

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@steveharter @ahsonkhan @jozkee