Add associated warning #373
Conversation
|
Looks like you've passed all of the checks! |
| @@ -46,20 +46,25 @@ jobs: | |||
| CONTENTS: ${{ steps.read_results.outputs.contents }} | |||
| run: echo "$CONTENTS" | |||
| - name: Create Comment | |||
| if: steps.read_results.outputs.contents != 'success' | |||
| if: steps.read_results.outputs.contents != 'success' && ${{ !startsWith(steps.read_results.outputs.contents, 'Warning')}} | |||
There was a problem hiding this comment.
Does this support emitting a warning alongside errors? What if the warning happens to be the first line that was emitted, but it's followed by errors?
(Maybe we ought to write separate files for warnings/errors.)
| - name: Write Comment on PR | ||
| uses: mshick/add-pr-comment@v2 | ||
| with: | ||
| message-path: "message.txt" | ||
| refresh-message-position: true | ||
| - name: Fail or Succeed | ||
| if: steps.read_results.outputs.contents != 'success' | ||
| if: startsWith(steps.read_results.outputs.contents, 'It appears you have failed') |
There was a problem hiding this comment.
Can all of this be moved into the "Create Comment" step, so that we don't have to duplicate the "It appears you have failed" substring? Duplicating that makes this brittle, it'll break if someone changes that string and doesn't realize there's a dependency here.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| Args: | ||
| check_sets: Dict[string, RwsSet] | ||
| Returns: |
There was a problem hiding this comment.
For future refactoring: IMO it'd be better design to make this a "pure" function, i.e. it takes all its inputs as arguments, and returns its result explicitly (rather than just mutating the instance). That makes it easier to test, and easier for a reader to understand how to use the method.
Doing that in this CL would require the caller to know whether the results should be considered warnings or errors, though, which isn't great. What'd be nice is if each check returned a list of errors and and list of warnings, and the caller just appended each to its running lists. That's a larger change, so it shouldn't be part of this PR.
| for checker_error in rws_checker.error_list: | ||
| print(checker_error) | ||
| for error_text in error_texts: | ||
| print(error_text) | ||
| else: | ||
| for warning in rws_checker.warning_texts: |
There was a problem hiding this comment.
Maybe add a comment here that indicates it's important for the warnings to be after errors? Unless you choose to remove that limitation, per my other comment.
| if rws_checker.error_list or error_texts or rws_checker.warning_texts: | ||
| for checker_error in rws_checker.error_list: | ||
| print(checker_error) | ||
| for error_text in error_texts: | ||
| print(error_text) | ||
| else: | ||
| for warning in rws_checker.warning_texts: | ||
| print(warning) |
There was a problem hiding this comment.
This is getting complicated/repetitive; consider extracting the things we want to print:
outputs = rws_checker.error_list + error_texts + rws_checker.warning_texts
if outputs:
for output in outputs:
print(output)
else:
print("success", end="")Note that this no longer repeats each list twice.
| icanns=set()) | ||
| loaded_sets = rws_check.load_sets() | ||
| rws_check.check_associated_count(loaded_sets) | ||
| self.assertEqual(rws_check.associated_warning, []) |
There was a problem hiding this comment.
This assertion is more specific than it needs to be, so it is a change-detector assertion. (If we add another kind of warning in the future, this assertion may fail even though the condition that it's meant to check -- that the >5 associated sites warning is not present -- is still true.)
Please check for the specific warning that this test cares about, not that there are 0 warnings overall.
| icanns=set()) | ||
| loaded_sets = rws_check.load_sets() | ||
| rws_check.check_associated_count(loaded_sets) | ||
| self.assertEqual(rws_check.associated_warning, |
There was a problem hiding this comment.
Please check that the warning we want is among the actual warnings; not that it is the only warning. (You could do this with self.assertIn or self.assertCountEqual.)
Same comment for the test case below.
| icanns=set()) | ||
| loaded_sets = rws_check.load_sets() | ||
| rws_check.check_associated_count(loaded_sets) | ||
| self.assertEqual(rws_check.associated_warning, []) |
There was a problem hiding this comment.
Where is rws_check.associated_warning defined? It looks like you meant this to be rws_check.warning_texts instead, here and below. If so, please investigate why the tests passed despite this typo?
As per #339, the checks should emit a warning when submissions contain more than 5 associated sites. This PR adds the warning to the automated comment left on PRs which informs submitters of success or failure.