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

Minor updates. #509

Merged
merged 2 commits into from
Jan 30, 2021
Merged

Minor updates. #509

merged 2 commits into from
Jan 30, 2021

Conversation

david-driscoll
Copy link
Member

  • Added Container.From
  • Do not serialize out the PrivateHandlerId if it's empty
  • StringOrMarkupContent implict conversion should be nullable.

cc @NTaylorMullen @333fred

…'s empty. StringOrMarkupContent implict conversion should be nullable.
@david-driscoll david-driscoll added the bug Something isn't working label Jan 28, 2021
@@ -17,9 +17,9 @@ public record StringOrMarkupContent
public MarkupContent? MarkupContent { get; }
public bool HasMarkupContent => String == null;

public static implicit operator StringOrMarkupContent(string value) => new StringOrMarkupContent(value);
public static implicit operator StringOrMarkupContent?(string? value) => value is null ? null : new StringOrMarkupContent(value);
Copy link

Choose a reason for hiding this comment

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

Could have NotNullIfNotNull

Copy link

Choose a reason for hiding this comment

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

@david-driscoll doesn't look like this was addressed.

@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #509 (97d474b) into master (1c5e7dc) will decrease coverage by 3.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
- Coverage   73.46%   69.85%   -3.61%     
==========================================
  Files         256      256              
  Lines       12246    12246              
  Branches      827      827              
==========================================
- Hits         8996     8554     -442     
- Misses       3250     3428     +178     
- Partials        0      264     +264     
Impacted Files Coverage Δ
src/Dap.Client/DebugAdapterClient.cs 96.77% <ø> (-3.23%) ⬇️
src/Server/Pipelines/ResolveCommandPipeline.cs 100.00% <100.00%> (ø)
src/JsonRpc/ExternalServiceProvider.cs 50.00% <0.00%> (-50.00%) ⬇️
src/JsonRpc/Server/Messages/InternalError.cs 33.33% <0.00%> (-33.34%) ⬇️
src/JsonRpc.Generators/SymbolExtensions.cs 71.42% <0.00%> (-28.58%) ⬇️
src/Dap.Client/ProgressObservable.cs 0.00% <0.00%> (-22.23%) ⬇️
src/JsonRpc/Server/ContentModifiedException.cs 37.50% <0.00%> (-18.75%) ⬇️
src/JsonRpc/Server/RequestCancelledException.cs 37.50% <0.00%> (-18.75%) ⬇️
...rver/Configuration/ChainedConfigurationProvider.cs 58.62% <0.00%> (-17.25%) ⬇️
...ient/Configuration/ChainedConfigurationProvider.cs 0.00% <0.00%> (-17.25%) ⬇️
... and 97 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c5e7dc...97d474b. Read the comment docs.

Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Excited to have a variant that doesn't require the serialization of the resolver handler ID, /cc @ToddGrun since we'll both probably want to adopt this for perf reasons.

@david-driscoll david-driscoll merged commit e68e6e3 into master Jan 30, 2021
@david-driscoll david-driscoll deleted the fixup/minor-changes branch January 30, 2021 16:26
@github-actions github-actions bot added this to the v0.19.0 milestone Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants