Skip to content

Conversation

@aajisaka
Copy link
Member

"audience": ("Public", "Private"),
"stability": ("Stable", "Evolving"),
"replaceable": ("yes", "no"),
"replaceable": ("Yes", "No"),
Copy link
Contributor

Choose a reason for hiding this comment

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

'Yes' and 'No' are not documented as being valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

def getreplace(self):
'''get the replacement state'''
if self.replaceb:
return "Yes"
return "No"

Here self.getreplace returns "Yes" or "No", therefore I changed the valid values correspondingly to pass the elif statement below:
elif value not in validvalues[attr]:
validvalue = "|".join(v.lower() for v in validvalues[attr])
messages.append(
"%s:%u: ERROR: function %s has invalid value (%s) for @%s (%s)"
% (self.getfilename(), self.getlinenum(), self.getname(),
value.lower(), attr.lower(), validvalue))

Should I update self.getreplace to return "yes/no" instead of "Yes/No"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rethinking this, just adding a function to get the raw replaceable text is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

The point of YETUS-1089 was to better enforce the ruleset and provide valid output. 'yes' or 'no' are the values that are considered valid. A value of 'Yes' or 'No' would be considered invalid and they should throw an error. Checking the validity of the rendered output rather than the parsed value doesn't do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @aw-was-here for the detail. I've updated the patch to check the parsed value. 0e3b97a

Copy link
Member Author

Choose a reason for hiding this comment

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

value.lower(), attr.lower(), validvalue))

You said the validity check should be case-sensitive, so I thought value.lower() should be value in the error message. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought value.lower() should be value in the error message.

Umm. It is not good for other attrs because the validity check of the attrs looks case-insensitive.

@aajisaka aajisaka requested a review from aw-was-here January 28, 2021 02:04
@aajisaka
Copy link
Member Author

@aw-was-here Would you check the latest commit? I think now it validates the parsed value.

@aajisaka
Copy link
Member Author

aajisaka commented Feb 5, 2021

I'll merge tomorrow if there are no objections.

@aw-was-here
Copy link
Contributor

aw-was-here commented Feb 6, 2021

Yeah, this might work. Might as well commit it and we'll deal with it later if it doesn't. (There are a ton of other bugs in 0.13 that are probably more visible soooo)

@aajisaka aajisaka merged commit 94857fb into apache:main Feb 8, 2021
@aajisaka aajisaka deleted the YETUS-1099 branch February 8, 2021 02:27
@aajisaka
Copy link
Member Author

aajisaka commented Feb 8, 2021

Thank you @aw-was-here and @busbey

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.

3 participants