Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Adjusted MsgPack004 to support records #1932

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

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

N-Olbert
Copy link
Contributor

Fixes #1924

Added corresponding tests.

Note: For records with a primary constructor we do not generate a code fix at the moment, since we can not specify the target of the generated attribute (since AttributeSyntax.WithTarget is missing).

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks very much for contributing this. We may have opportunities to improve this further. What do you think?

src/MessagePackAnalyzer/MessagePackCodeFixProvider.cs Outdated Show resolved Hide resolved
src/MessagePackAnalyzer/MessagePackCodeFixProvider.cs Outdated Show resolved Hide resolved
@AArnott
Copy link
Collaborator

AArnott commented Aug 19, 2024

Ooh, I have another request for you. This change doesn't merge cleanly with develop, which has a lot of changes because we're totally reworking the analyzer to include a source generator. I'd like to get this fixed in develop as well (assuming the bug also repros there) so that your fix carries forward to our v3 version.
After this PR merges, would you be willing to merge develop into your topic branch, resolve conflicts, and submit another PR to our develop branch?

@AArnott AArnott added this to the v2.5 milestone Aug 19, 2024
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks!

@AArnott AArnott merged commit fc3cada into MessagePack-CSharp:master Aug 19, 2024
5 of 6 checks passed
@N-Olbert
Copy link
Contributor Author

Ooh, I have another request for you. This change doesn't merge cleanly with develop, which has a lot of changes because we're totally reworking the analyzer to include a source generator. I'd like to get this fixed in develop as well (assuming the bug also repros there) so that your fix carries forward to our v3 version. After this PR merges, would you be willing to merge develop into your topic branch, resolve conflicts, and submit another PR to our develop branch?

Will do later this week 👍 Thanks for 'primary constructor attributes'-fix.

@N-Olbert
Copy link
Contributor Author

I did not forget about this. I will merge it into develop within the next week, I was busy until now.

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.

MessagePack.Analyzers / MsgPack004 not working for records
2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.