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

Replaced SingleAsArray with OutputFormat = JsonArray + JsonDictionary #697

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Sep 16, 2021

Affects the following layout-renderers:

  • AspNetRequestCookieLayoutRenderer
  • AspNetRequestFormLayoutRenderer
  • AspNetRequestHeadersLayoutRenderer
  • AspNetQueryStringLayoutRenderer

I'm open for different property-name that gives better meaning. Very tempted to introduce OutputFormat=JsonDictionary

@304NotModified
Copy link
Member

is it now impossible to render a single item as an array? Why is that? I don't see the rationale for this change,

Very tempted to introduce OutputFormat=JsonDictionary

That would be indeed great, also If we could have OutputFormat=JsonArray

@snakefoot
Copy link
Contributor Author

is it now impossible to render a single item as an array? Why is that? I don't see the rationale for this change,

ValuesAsProperties = false means always as array, even when single-item.

@304NotModified
Copy link
Member

304NotModified commented Sep 25, 2021

ah so a single item without array or object is now not possible?

@snakefoot
Copy link
Contributor Author

snakefoot commented Sep 25, 2021

ah so a single item without array or object is now not possible?

  • SingleAsArray = false - Means render key-value-pair as Json-Dictionary, but only when single item.
  • SingleAsArray = true - Means render key-value-pair as Json-Array independent of item-count.

Now it is changed so you always get Json-Dictionary or Json-Array. Not a mixture of both depending on item-count.

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2021

Codecov Report

Merging #697 (0f8f846) into master (0990a06) will decrease coverage by 0%.
The diff coverage is 70%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #697   +/-   ##
=====================================
- Coverage      60%    60%   -0%     
=====================================
  Files          40     40           
  Lines         668    671    +3     
  Branches      161    163    +2     
=====================================
- Hits          404    403    -1     
- Misses        208    212    +4     
  Partials       56     56           
Impacted Files Coverage Δ
...outRenderers/AspNetRequestHeadersLayoutRenderer.cs 81% <ø> (ø)
...enderers/AspNetRequestQueryStringLayoutRenderer.cs 82% <ø> (ø)
...outRenderers/AspNetLayoutMultiValueRendererBase.cs 83% <68%> (-5%) ⬇️
...youtRenderers/AspNetRequestCookieLayoutRenderer.cs 87% <100%> (ø)

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 0990a06...0f8f846. Read the comment docs.

@snakefoot snakefoot changed the title Introduced ValuesAsProperties to replace SingleAsArray Replaced SingleAsArray with OutputFormat = JsonArray + JsonDictionary Sep 25, 2021
@snakefoot snakefoot closed this Oct 28, 2021
@snakefoot snakefoot reopened this Oct 28, 2021
@snakefoot snakefoot removed the request for review from 304NotModified October 28, 2021 21:11
@snakefoot snakefoot force-pushed the master branch 5 times, most recently from eba1225 to 1d63e51 Compare October 28, 2021 21:41
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

89.1% 89.1% Coverage
0.0% 0.0% Duplication

@snakefoot snakefoot merged commit cf6405c into NLog:master Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants