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

Issue 2211. ScalePrecisionValidator error message clarification. #2213

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dropik
Copy link

@dropik dropik commented May 14, 2024

As described in issue #2211, clarified PrecisionScale validator behaviour in docs. Provided a workaround to avoid self contradictory error messages. Also noted inconsistent russian translation of error message in confront to other translations.

@dropik
Copy link
Author

dropik commented May 14, 2024

@dotnet-policy-service agree
@dotnet-policy-service agree

@JeremySkinner
Copy link
Member

Thanks for putting together a PR @dropik. It'll take me some time to review this and consider the implications.

hi @nhowell I don't suppose you could take a look at this if you get some time? I know you did some work on the ScalePrecisionValidator before and would appreciate your input

Copy link
Contributor

@nhowell nhowell left a comment

Choose a reason for hiding this comment

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

I agree with the changes to the error message. Makes sense.

I provided some suggestions on the wording of a code comment that I found a bit clunky, but otherwise this looks good to me.

Co-authored-by: Nick Howell <howellnick@gmail.com>
@dropik
Copy link
Author

dropik commented May 18, 2024

@JeremySkinner , @nhowell , thank you for your time! @nhowell , yes this looks better, thank you for suggestion!

@JeremySkinner
Copy link
Member

Thanks! I'll aim to get this merged in the next time I've got time to work on FV

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.

None yet

3 participants