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.

Sign up for GitHub

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

Jump to bottom

Diagnostics for module initializers feature #43301

Merged
merged 37 commits into from
May 12, 2020
Merged

Diagnostics for module initializers feature #43301

merged 37 commits into from
May 12, 2020

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Apr 12, 2020 •
edited
Loading

Test plan: #40500

@RikkiGibson On combining diagnostic IDs: I didn't make any changes as of now. I have a preference for not combining the messages themselves ("either X or Y is wrong"). Because of the way localization works, I don't know if it's possible to vary the message wording for the same diagnostic ID.

@RikkiGibson
Copy link
Contributor

On combining diagnostic IDs: I didn't make any changes as of now. I have a preference for not combining the messages themselves ("either X or Y is wrong"). Because of the way localization works, I don't know if it's possible to vary the message wording for the same diagnostic ID.

The unit of translation is usually a single diagnostic message. We sometimes parameterize based on simple discriminators e.g. saying "type" versus "method", but nothing more complex than that. I'm happy with your preference, so feel free to leave the diagnostics as is :)

@RikkiGibson RikkiGibson added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 13, 2020
@RikkiGibson RikkiGibson added this to the Compiler.Net5 milestone Apr 13, 2020
@RikkiGibson
Copy link
Contributor

Happy to review whenever this is ready.

@jnm2 jnm2 force-pushed the module_initializers/diagnostics branch from c7c5a81 to 0f3f43a Compare April 15, 2020 02:38
@jnm2
Copy link
Contributor Author

jnm2 commented Apr 15, 2020

@RikkiGibson It's now ready to be reviewed whenever #43281 merges. If you prefer, you could review before that if you want to review by commit and ignore the commits before the first commit for this PR, "Planned diagnostics."

@RikkiGibson
Copy link
Contributor

If this is ready to review, could you please change the base to features/module-initializers and consider rebasing this PR to just commits b8e0f42 through 0f3f43a.

@jnm2 jnm2 changed the base branch from master to features/module-initializers April 29, 2020 02:42
@jnm2 jnm2 force-pushed the module_initializers/diagnostics branch from 0f3f43a to 3255afa Compare April 29, 2020 02:42
@jnm2 jnm2 changed the title [🚧] Diagnostics for module initializers feature Diagnostics for module initializers feature Apr 29, 2020
@jnm2 jnm2 marked this pull request as ready for review April 29, 2020 02:42
@jnm2 jnm2 requested a review from a team as a code owner April 29, 2020 02:42
@jnm2
Copy link
Contributor Author

jnm2 commented Apr 29, 2020

@RikkiGibson Thanks, I hadn't realized I had the wrong base. Rebased and ready for review!

RikkiGibson
RikkiGibson previously approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

nice work! Just had a few nits to pick.

src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.Accessibility.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.Accessibility.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.Accessibility.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.Ignored.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.Targets.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.Targets.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs Outdated Show resolved Hide resolved
@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 8, 2020 •
edited
Loading

Would you still like me to move the test files into a folder?

I would do that, but it is your call. However, I think we should at least change file names to not have a dot in them, i.e. replace the dot with _. I think that is the convention that we are following. #Closed

RikkiGibson
RikkiGibson reviewed May 8, 2020
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/ModuleInitializersTests.Targets.cs Outdated
);
}
}
}
Copy link
Contributor

@RikkiGibson RikkiGibson May 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Consider testing what happens when the target of the [ModuleInitializerAttribute] is the definition part of a static partial method with no implementation part.

Consider testing what happens when the target of the [ModuleInitializerAttribute] is the implementation part of a static partial method.

edit: actually, these tests are probably moot for now since the partial methods must be 'private'

Copy link
Contributor Author

@jnm2 jnm2 May 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, though public partial methods in language design right now IIRC. Since I couldn't test that yet, I skipped it.

@RikkiGibson
Copy link
Contributor

Done with review pass (iteration 31).

@jnm2 jnm2 force-pushed the module_initializers/diagnostics branch from 0d7183f to 64269a9 Compare May 8, 2020 22:46
@jnm2 jnm2 requested review from RikkiGibson and AlekseyTs May 8, 2020 23:08
RikkiGibson
RikkiGibson approved these changes May 9, 2020
View reviewed changes
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 35)

@jnm2
Copy link
Contributor Author

jnm2 commented May 9, 2020

@AlekseyTs @RikkiGibson I switched to a folder instead of a partial class. Ready for squash merge :)

RikkiGibson
RikkiGibson approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

I'm OK with this test organization.

@RikkiGibson
Copy link
Contributor

@AlekseyTs I don't see an official sign off from you, please take another look and if you have no further feedback/concerns then we can merge.

@AlekseyTs
Copy link
Contributor

Done with review pass (iteration 36)

@jnm2 jnm2 requested a review from AlekseyTs May 12, 2020 02:13
AlekseyTs
AlekseyTs approved these changes May 12, 2020
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 37)

@AlekseyTs AlekseyTs merged commit 8145427 into dotnet:features/module-initializers May 12, 2020
@AlekseyTs
Copy link
Contributor

@jnm2 Thanks for the contribution.

@jnm2 jnm2 deleted the module_initializers/diagnostics branch May 12, 2020 15:27
@jnm2
Copy link
Contributor Author

jnm2 commented May 12, 2020

Thanks everyone for the help!

@jnm2 jnm2 mentioned this pull request May 12, 2020
Closed
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredpar jaredpar jaredpar left review comments

@cston cston cston left review comments

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
Compiler.Net5
Development

Successfully merging this pull request may close these issues.
5 participants
@jnm2 @RikkiGibson @AlekseyTs @jaredpar @cston