You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@yetus.apache.org by GitBox <gi...@apache.org> on 2021/01/26 11:22:00 UTC
[GitHub] [yetus] aajisaka opened a new pull request #212: YETUS-1099. shelldocs --lint option fails
aajisaka opened a new pull request #212:
URL: https://github.com/apache/yetus/pull/212
JIRA: https://issues.apache.org/jira/browse/YETUS-1099
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on a change in pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on a change in pull request #212:
URL: https://github.com/apache/yetus/pull/212#discussion_r564988758
##########
File path: shelldocs/src/main/python/shelldocs.py
##########
@@ -261,12 +261,12 @@ def lint(self):
getfuncs = {
"audience": self.getaudience,
"stability": self.getstability,
- "replaceable": self.replacetext,
+ "replaceable": self.getreplace,
}
validvalues = {
"audience": ("Public", "Private"),
"stability": ("Stable", "Evolving"),
- "replaceable": ("yes", "no"),
+ "replaceable": ("Yes", "No"),
Review comment:
Thank you @aw-was-here for the detail. I've updated the patch to check the parsed value. 0e3b97a
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on pull request #212:
URL: https://github.com/apache/yetus/pull/212#issuecomment-773784285
I'll merge tomorrow if there are no objections.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on a change in pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on a change in pull request #212:
URL: https://github.com/apache/yetus/pull/212#discussion_r564992223
##########
File path: shelldocs/src/main/python/shelldocs.py
##########
@@ -261,12 +261,12 @@ def lint(self):
getfuncs = {
"audience": self.getaudience,
"stability": self.getstability,
- "replaceable": self.replacetext,
+ "replaceable": self.getreplace,
}
validvalues = {
"audience": ("Public", "Private"),
"stability": ("Stable", "Evolving"),
- "replaceable": ("yes", "no"),
+ "replaceable": ("Yes", "No"),
Review comment:
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aw-was-here commented on a change in pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aw-was-here commented on a change in pull request #212:
URL: https://github.com/apache/yetus/pull/212#discussion_r564980273
##########
File path: shelldocs/src/main/python/shelldocs.py
##########
@@ -261,12 +261,12 @@ def lint(self):
getfuncs = {
"audience": self.getaudience,
"stability": self.getstability,
- "replaceable": self.replacetext,
+ "replaceable": self.getreplace,
}
validvalues = {
"audience": ("Public", "Private"),
"stability": ("Stable", "Evolving"),
- "replaceable": ("yes", "no"),
+ "replaceable": ("Yes", "No"),
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aw-was-here edited a comment on pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aw-was-here edited a comment on pull request #212:
URL: https://github.com/apache/yetus/pull/212#issuecomment-774394846
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aw-was-here commented on a change in pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aw-was-here commented on a change in pull request #212:
URL: https://github.com/apache/yetus/pull/212#discussion_r564709247
##########
File path: shelldocs/src/main/python/shelldocs.py
##########
@@ -261,12 +261,12 @@ def lint(self):
getfuncs = {
"audience": self.getaudience,
"stability": self.getstability,
- "replaceable": self.replacetext,
+ "replaceable": self.getreplace,
}
validvalues = {
"audience": ("Public", "Private"),
"stability": ("Stable", "Evolving"),
- "replaceable": ("yes", "no"),
+ "replaceable": ("Yes", "No"),
Review comment:
'Yes' and 'No' are not documented as being valid.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on a change in pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on a change in pull request #212:
URL: https://github.com/apache/yetus/pull/212#discussion_r564727194
##########
File path: shelldocs/src/main/python/shelldocs.py
##########
@@ -261,12 +261,12 @@ def lint(self):
getfuncs = {
"audience": self.getaudience,
"stability": self.getstability,
- "replaceable": self.replacetext,
+ "replaceable": self.getreplace,
}
validvalues = {
"audience": ("Public", "Private"),
"stability": ("Stable", "Evolving"),
- "replaceable": ("yes", "no"),
+ "replaceable": ("Yes", "No"),
Review comment:
https://github.com/apache/yetus/blob/227c762a0154c3465923d3b59ebdb3063bbd95ca/shelldocs/src/main/python/shelldocs.py#L174-L178
Here `self.getreplace` returns "Yes" or "No", therefore I changed the valid values correspondingly to pass the `elif` statement below:
https://github.com/apache/yetus/blob/227c762a0154c3465923d3b59ebdb3063bbd95ca/shelldocs/src/main/python/shelldocs.py#L278-L283
Should I update `self.getreplace` to return "yes/no" instead of "Yes/No"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on a change in pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on a change in pull request #212:
URL: https://github.com/apache/yetus/pull/212#discussion_r564990737
##########
File path: shelldocs/src/main/python/shelldocs.py
##########
@@ -261,12 +261,12 @@ def lint(self):
getfuncs = {
"audience": self.getaudience,
"stability": self.getstability,
- "replaceable": self.replacetext,
+ "replaceable": self.getreplace,
}
validvalues = {
"audience": ("Public", "Private"),
"stability": ("Stable", "Evolving"),
- "replaceable": ("yes", "no"),
+ "replaceable": ("Yes", "No"),
Review comment:
https://github.com/apache/yetus/blob/227c762a0154c3465923d3b59ebdb3063bbd95ca/shelldocs/src/main/python/shelldocs.py#L283
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on pull request #212:
URL: https://github.com/apache/yetus/pull/212#issuecomment-773784285
I'll merge tomorrow if there are no objections.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on pull request #212:
URL: https://github.com/apache/yetus/pull/212#issuecomment-769618697
@aw-was-here Would you check the latest commit? I think now it validates the parsed value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on pull request #212:
URL: https://github.com/apache/yetus/pull/212#issuecomment-774825539
Thank you @aw-was-here and @busbey
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka merged pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka merged pull request #212:
URL: https://github.com/apache/yetus/pull/212
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aw-was-here commented on pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aw-was-here commented on pull request #212:
URL: https://github.com/apache/yetus/pull/212#issuecomment-774394846
Yeah, this might work. Might as well commit it and we'll deal with it later if it doesn't.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yetus] aajisaka commented on a change in pull request #212: YETUS-1099. shelldocs --lint option fails
Posted by GitBox <gi...@apache.org>.
aajisaka commented on a change in pull request #212:
URL: https://github.com/apache/yetus/pull/212#discussion_r564736904
##########
File path: shelldocs/src/main/python/shelldocs.py
##########
@@ -261,12 +261,12 @@ def lint(self):
getfuncs = {
"audience": self.getaudience,
"stability": self.getstability,
- "replaceable": self.replacetext,
+ "replaceable": self.getreplace,
}
validvalues = {
"audience": ("Public", "Private"),
"stability": ("Stable", "Evolving"),
- "replaceable": ("yes", "no"),
+ "replaceable": ("Yes", "No"),
Review comment:
Rethinking this, just adding a function to get the raw replaceable text is fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org