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

Fix: DelegateParameter.validate also validates the source parameter #4870

Conversation

Junliang-Wang
Copy link
Contributor

Prior to this change, DelegateParameter.validate only uses its own validator, ignoring the source parameter.

This change adds an extra layer of validation by using the source parameter's validator in DelegateParameter.validate.

These changes are related to the discussion #4819

Junliang-Wang and others added 2 commits December 15, 2022 14:41
Prior to this change, DelegateParameter.validate only uses its own
validator, ignoring the source parameter.

This change adds an extra layer of validation by using the source
parameter's validator in DelegateParameter.validate.
@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #4870 (2e42a5c) into master (8ede8c2) will decrease coverage by 0.90%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4870      +/-   ##
==========================================
- Coverage   68.31%   67.41%   -0.91%     
==========================================
  Files         340      340              
  Lines       32236    32240       +4     
==========================================
- Hits        22022    21733     -289     
- Misses      10214    10507     +293     

Junliang-Wang and others added 4 commits December 16, 2022 17:24
Prior to this change, it was missing a test when Parameter or
DelegateParameter has offset and scale

This change adds a test to verify that .validate ignores the offset and
scale
Prior to this change, DelegateParameter.validate only uses its own
validator, ignoring the source parameter.

This change adds an extra layer of validation by using the source
parameter's validator in DelegateParameter.validate.
Prior to this change, it was missing a test when Parameter or
DelegateParameter has offset and scale

This change adds a test to verify that .validate ignores the offset and
scale
@jenshnielsen jenshnielsen force-pushed the hotfix/DelegateParamValueValidation branch from f364667 to 2ac5285 Compare December 22, 2022 09:44
@jenshnielsen
Copy link
Collaborator

@Junliang-Wang Thanks, Could you add a small description of the change in docs/changes/newsfragments/4870.improved

@Junliang-Wang
Copy link
Contributor Author

@Junliang-Wang Thanks, Could you add a small description of the change in docs/changes/newsfragments/4870.improved

Sure, although I have a technical question: my local branch is now behind the master, I suppose that I should pull for the updates, and then commit the file in docs/changes/newsfragments/4870.improved, right? If I do so, I see that there is no 4870.improved, but rather 4876.improved. Should I create 4870.improved and write my changes?

@astafan8
Copy link
Contributor

Sure, although I have a technical question: my local branch is now behind the master, I suppose that I should pull for the updates, and then commit the file in docs/changes/newsfragments/4870.improved, right? If I do so, I see that there is no 4870.improved, but rather 4876.improved. Should I create 4870.improved and write my changes?

@Junliang-Wang "Should I create 4870.improved and write my changes" - yes :) these change-files are written PER pull-request, so don't add information to the existing ones :) So, the 4876.improved file that you see was written for PR number 4876, and the one you're about to write is for 4870 because the PR you have open has number 4870.

@Junliang-Wang
Copy link
Contributor Author

@astafan8 Thanks for the answer! I have pushed your suggestion, along with 4870.improved file.

@jenshnielsen jenshnielsen merged commit 5dcaf36 into microsoft:master Jan 5, 2023
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