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 ConstantExpectedAttribute #62436

Merged
merged 5 commits into from
Dec 15, 2021

Conversation

wzchua
Copy link
Contributor

@wzchua wzchua commented Dec 6, 2021

Contributes to #33771

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 6, 2021
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@GrabYourPitchforks
Copy link
Member

Thanks for the tests as well!

@stephentoub - What do you make of the idea of adding unit tests like the below?

var attr = new ConstantExpectedAttribute { Min = 10, Max = 0 };
// assert blah blah

It's clearly incorrect to write that, but it's nonsense that should be caught at design time and not as part of the attribute's own logic. Would it ever make sense to include this as an "it's nonsense but the attribute setters shouldn't block this" check?

@stephentoub
Copy link
Member

What do you make of the idea of adding unit tests like the below?

Seems reasonable.

@ghost
Copy link

ghost commented Dec 13, 2021

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #33771

Author: wzchua
Assignees: -
Labels:

area-System.Diagnostics, new-api-needs-documentation, community-contribution

Milestone: -

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Thanks @wzchua, LGTM

@buyaa-n buyaa-n merged commit af726fc into dotnet:main Dec 15, 2021
@stephentoub
Copy link
Member

stephentoub commented Jan 3, 2022

I assume there are existing APIs, in particular in intrinsics, where this should be used? #30740 (comment) calls out some. Is there any issue tracking attributing those parameters? In general my preference is to use new APIs in the same PRs that add those APIs, in order to help validate them. As this has already been merged, it could be done in a follow-up.
cc: @GrabYourPitchforks

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 6, 2022

Is there any issue tracking attributing those parameters?

Sure I was planning to create an issue for tracking the APIs needs this, created #63426

In general my preference is to use new APIs in the same PRs that add those APIs, in order to help validate them. As this has already been merged, it could be done in a follow-up.

Understand, for this one it was not clear what parameters of what API's needs this except the one you mentioned, hope @tannergooding would add the APIs to the new issue. Merged the PR so that it could be able to be used for the analyzer as soon as getting available for roslyn-analyzers repo.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants