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

Device and gpu protocols updated; public IJsonSerializable #1063

Merged
merged 11 commits into from
Jun 18, 2021
Merged

Device and gpu protocols updated; public IJsonSerializable #1063

merged 11 commits into from
Jun 18, 2021

Conversation

semuserable
Copy link
Contributor

@semuserable semuserable commented Jun 17, 2021

Related to getsentry/sentry-unity#49, check latest comment (which starts with 'Proposed changes').

I made IJsonSerializable public, because we'll have a new context on Unity side. Although, I'm not sure about summary description.

I also plan to update the docs, but after this part will be merged.

I plan to update the tests too. Still WIP.

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2021

Codecov Report

Merging #1063 (8dadaca) into main (2ac97c8) will increase coverage by 1.70%.
The diff coverage is 83.80%.

❗ Current head 8dadaca differs from pull request most recent head 07e32a3. Consider uploading reports for the commit 07e32a3 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1063      +/-   ##
==========================================
+ Coverage   79.52%   81.22%   +1.70%     
==========================================
  Files         194      194              
  Lines        6349     6451     +102     
  Branches     1530     1564      +34     
==========================================
+ Hits         5049     5240     +191     
+ Misses        868      758     -110     
- Partials      432      453      +21     
Impacted Files Coverage Δ
src/Sentry/IJsonSerializable.cs 64.70% <ø> (ø)
src/Sentry/Protocol/Gpu.cs 85.00% <83.78%> (-0.94%) ⬇️
src/Sentry/Protocol/Device.cs 85.59% <83.82%> (-0.88%) ⬇️
src/Sentry/Internal/AppDomainAdapter.cs 66.66% <0.00%> (ø)
src/Sentry/Internal/BackgroundWorker.cs 84.93% <0.00%> (+0.68%) ⬆️
src/Sentry/GlobalSessionManager.cs 65.78% <0.00%> (+2.63%) ⬆️
...ry/Internal/Http/DefaultSentryHttpClientFactory.cs 62.50% <0.00%> (+4.16%) ⬆️
src/Sentry/PlatformAbstractions/RuntimeInfo.cs 58.62% <0.00%> (+5.17%) ⬆️
...ntry/PlatformAbstractions/FrameworkInstallation.cs 62.50% <0.00%> (+37.50%) ⬆️
...ntry/Integrations/NetFxInstallationsIntegration.cs 100.00% <0.00%> (+66.66%) ⬆️
... and 4 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 2ac97c8...07e32a3. Read the comment docs.

@semuserable
Copy link
Contributor Author

semuserable commented Jun 18, 2021

Added the tests. Please, review @Tyrrrz @bruno-garcia as I'd like to start the integration on Unity side for these protocols.

Also, why danger fails again?

@lucas-zimerman
Copy link
Collaborator

Please consider adding a changelog entry for the next release.

Instructions and example for changelog
Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:
Device and gpu protocols updated; public IJsonSerializable([#1063](https://github.com/getsentry/sentry-dotnet/pull/1063))
If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

@lucas-zimerman
Copy link
Collaborator

Perhaps using const names for the field names, for example:

internal const string ProcessorCountKey = "processor_count";
...
if (ProcessorCount is {} processorCount)           
 {                
writer.WriteNumber(ProcessorCountKey , processorCount);           
 }
...
var processorCount = json.GetPropertyOrNull(ProcessorCountKey)?.GetInt32();

Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
@semuserable
Copy link
Contributor Author

semuserable commented Jun 18, 2021

Perhaps using const names for the field names, for example:

internal const string ProcessorCountKey = "processor_count";
...
if (ProcessorCount is {} processorCount)           
 {                
writer.WriteNumber(ProcessorCountKey , processorCount);           
 }
...
var processorCount = json.GetPropertyOrNull(ProcessorCountKey)?.GetInt32();

I was relying on the current style. I haven't seen such usage from within the project, so used directly.

semuserable and others added 2 commits June 18, 2021 18:39
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
@bruno-garcia bruno-garcia merged commit 9b32eee into getsentry:main Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants