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

chore: Refactor docfx command related code #9140

Merged
merged 3 commits into from
Sep 1, 2023

Conversation

filzrev
Copy link
Contributor

@filzrev filzrev commented Aug 28, 2023

What's included in this PR

  • Change docfx command related API/Models from internal to public.

  • Remove <InternalsVisibleTo Include="docfx" /> settings

  • Move constant string docfx.json definition to Docfx.DataContracts.Common project.

  • Add RunMetadata.cs wrapper to use DotnetApiCatalog API.

Background
As described at #8872.
Current Docfx.App package expose limited API set only.
By this PR, Docfx.App user can consume same APIs that is used by docfx.exe.

@yufeih yufeih added the new-feature Makes the pull request to appear in "New Features" section of the next release note label Aug 31, 2023
@yufeih
Copy link
Contributor

yufeih commented Aug 31, 2023

Thanks for putting this together. I need some time to examine the public API surface and may likely tweak it a little bit.

@yufeih
Copy link
Contributor

yufeih commented Aug 31, 2023

Here are some of the public API guidelines for the config objects I have in mind:

  1. The C# property name should match the JSON config name, we could enforce it by applying [JsonObject(NamingStrategyType = typeof(CamelCaseNamingStrategy))] at the class level. We also don't seem to need the [Serializable] attributes.
  2. Internalize all obsolete properties
  3. Make the config object immutable so we don't need to make defensive copies or verify mutability:
    • Use init over set
    • Use ImmutableArray<T> over List<T> or T[]
    • Use ImmutableDictionary<TKey, TValue> over Dictionary<TKey, TValue>
  4. For types that allows both short-hand and expanded forms, expose only the expanded form with a custom JSON converter applied at property level?
    • ListWithStringFallback -> ImmutableArray<string>
    • FileMetadataPairs -> ImmutableDictionary<string, object>
    • FileItems -> ImmutableArray<string>
    • FileMapping -> ImmutableArray<FileMappingItem>

@filzrev filzrev force-pushed the feat-add-pass-configs-as-parameters branch from 25ddd3d to 8d70d47 Compare August 31, 2023 12:03
@filzrev filzrev force-pushed the feat-add-pass-configs-as-parameters branch from 8d70d47 to 4a3f37d Compare August 31, 2023 12:05
@filzrev
Copy link
Contributor Author

filzrev commented Aug 31, 2023

Thank you for your explanations about public API guidelines.
But 3. and 4. requires additional codes and it seems hard to maintenance.

So I decided to use IgnoresAccessChecksToGenerator to ignore internal modifier on my code.
And modify this PR with following changes only.

  • Move constant string docfx.json definition to Docfx.DataContracts.Common project.
  • Add RunMetadata.cs wrapper to use DotnetApiCatalog API.

@filzrev filzrev changed the title feat: Change docfx command related APIs/Models from internal to public chore: Refactor docfx command related code Aug 31, 2023
Copy link
Contributor

@yufeih yufeih left a comment

Choose a reason for hiding this comment

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

Thank you @filzrev

@yufeih yufeih added engineering Makes the pull request to appear in the "Engineering" section of the next release note and removed new-feature Makes the pull request to appear in "New Features" section of the next release note labels Sep 1, 2023
@yufeih yufeih merged commit e7366e2 into dotnet:main Sep 1, 2023
p-kostov pushed a commit to ErpNetDocs/docfx that referenced this pull request Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering Makes the pull request to appear in the "Engineering" section of the next release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants