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

Add ReportAssembliesMode in favor of ReportAssemblies #1079

Merged
merged 7 commits into from
Jun 23, 2021

Conversation

GeertvanHorrik
Copy link
Contributor

Fixes #1072

Introduces the InformationalVersionAttribute in favor of ReportAssemblies, covered by unit tests.

A few additional changes:

  1. Added additional null check for assemblies
  2. Moved retrieving the informational version attribute to try/catch since it was failing on the unit tests for NET461

Technically the GetName() can be optimized inside the foreach loop (only needed when value = in the MainSentryEventProcessor, but wasn't sure if it was worth less-readable code vs this optimization. Let me know if you would like this to be changed. For now this value is also used to determine whether an assembly should be checked / added at all.

Looking forward to your review. Have a great day!

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Perfect. Thanks!

@bruno-garcia
Copy link
Member

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

Example:

## Unreleased

### Features

* Add ReportAssembliesMode in favor of ReportAssemblies([#1079](https://github.com/getsentry/sentry-dotnet/pull/1079))

@GeertvanHorrik
Copy link
Contributor Author

Done, thanks for the fast review!

@bruno-garcia
Copy link
Member

bruno-garcia commented Jun 23, 2021

Seems like we have some failing tests @GeertvanHorrik

image

Can you repro them locally if you run all tests?

@GeertvanHorrik
Copy link
Contributor Author

That's my bad, sorry. I though returning string.Empty is better than null to prevent potential null reference exceptions. Will fix.

@GeertvanHorrik
Copy link
Contributor Author

Turning the tests back into their original behavior, but I am not sure if they are correct. At the moment, the informational version returns string.Empty (""), and that is being respected as null version. But I think it should fallback to the real assembly version in such case?

Maybe that's something to think about for a separate ticket. I will revert the old behavior back and respect a string.empty return from the informational version.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

I spotted one thing we might need to change

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Sorry for adding more comments but I think we should consider one more thing

src/Sentry/Reflection/AssemblyExtensions.cs Show resolved Hide resolved
@bruno-garcia bruno-garcia enabled auto-merge (squash) June 23, 2021 18:07
@bruno-garcia bruno-garcia merged commit a02001a into getsentry:main Jun 23, 2021
@bruno-garcia
Copy link
Member

@GeertvanHorrik Thanks for your contributions. I'll change the minor items I suggested and release it

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.

InformationalVersion for referenced assemblies
2 participants