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

Readonly members more test coverage #34602

Conversation

RikkiGibson
Copy link
Contributor

This PR contains test coverage additions mainly from the test plan review we had yesterday.

  • Check parser behavior for bad readonly property int P readonly => 42;
  • Improves warning message for implicit value copies
  • Require both property accessors be declared to use readonly keyword on an accessor (this matches the behavior with accessibility modifiers)

This is the last work I expect to do before finally merging master into this branch, and then integrating this branch into master.

@RikkiGibson RikkiGibson requested a review from a team as a code owner March 29, 2019 21:39
@RikkiGibson RikkiGibson requested a review from a team April 1, 2019 16:53
@jcouv
Copy link
Member

jcouv commented Apr 1, 2019

This is the last work I expect to do before finally merging master into this branch

Did you do an IDE test pass to identify what works and what doesn't? It's generally ok to file issues, except for few blockers: mostly crashes, annoying completion (ie. you can't type what you want and have to fight against the IDE), or any really annoying behavior.

@RikkiGibson
Copy link
Contributor Author

I've played with the feature in the IDE, authoring and referencing readonly members similar to what's written in the speclet, and found it worked pretty smoothly.

What's missing is generally what I've listed under the IDE section in #32911. Maybe that section should be migrated to a new issue?

@jcouv
Copy link
Member

jcouv commented Apr 1, 2019

Sounds good.

Maybe that section should be migrated to a new issue?

I would file individual issues. That isn't blocking the merge.

Copy link
Member

@jcouv jcouv 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 5) with one test suggestion

@RikkiGibson
Copy link
Contributor Author

I'm adding in a minor rewrite of an error message as discussed with @cston and @agocke.

@RikkiGibson RikkiGibson merged commit 0d15dd1 into dotnet:features/readonly-members Apr 1, 2019
@RikkiGibson RikkiGibson deleted the readonly-members-more-coverage branch April 1, 2019 23:12
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