Fix local testing workflow #162
Conversation
check_sites.py
Outdated
| input_prefix = '' | ||
| with_diff = False | ||
| opts, _ = getopt.getopt(args, "i:", ["data_directory=", "with_diff"]) | ||
| opts, _ = getopt.getopt(args, "i:", ["data_directory=", "with_diff", |
There was a problem hiding this comment.
Want to allow people to abbreviate this with a short option as well?
Getting-Started.md
Outdated
|
|
||
| 1. Run `cp related_website_sets.JSON my_rws.JSON` in your shell | ||
| `python3 check_sites.py --testing_sets=<your primary site>` |
There was a problem hiding this comment.
A bit of a naming nit, but the "testing" part feels redundant to me, and the "sets" part feels wrong because we're actually asking people to list the primary sites, not sets. So how about:
| `python3 check_sites.py --testing_sets=<your primary site>` | |
| `python3 check_sites.py --primary=<your primary site>` |
Getting-Started.md
Outdated
|
|
||
| 4. When you are ready to submit, copy your changes from `my_rws.JSON` into `related_website_sets.JSON` and delete `my_rws.JSON` | ||
| `python3 check_sites.py --testing_sets=<primary 1>,<primary 2>,<etc.>` |
There was a problem hiding this comment.
| `python3 check_sites.py --testing_sets=<primary 1>,<primary 2>,<etc.>` | |
| `python3 check_sites.py --testing_sets="https://foo.example,https://bar.example"` |
I think it would be helpful to show a more concrete example, so that people don't have to guess whether we require https://, whether quotes are necessary, etc.
check_sites.py
Outdated
| error_texts.append("There was an error loading the set:\n" + testing_set + | ||
| " could not be found in related_website_sets.JSON") |
There was a problem hiding this comment.
I think it might be better to print an error and quit entirely here, since the CLI arg was invalid. I'd rather force people to get the CLI arg completely right, than to have them see some of the output and (incorrectly) assume everything was fine.
There was a problem hiding this comment.
As I understand, you're saying in the case where a user enters multiple primaries and one of them is not in the RWS list, we should exit without giving error information about the sets whose primaries they entered correctly? If so, I think I disagree - I'd prefer we give us much error information as possible when they run the command. My guess is that most users are only going to be running the command with a single primary argument, so this would be moot anyway.
There was a problem hiding this comment.
Agree that it's probably rare that people will supply more than one site.
My fear about showing as much error information as possible in this case is that I think it would be easy to miss this error, in the midst of others. But I guess if there are other errors, then the dev will still know that there's something they need to fix; so if everything else is fine (and there is no other output), then it'll be easy to spot this error, anyway. So I'm ok with making this error non-fatal.
check_sites.py
Outdated
| error_texts.append("There was an error loading the set:\n" + testing_set + | ||
| " could not be found in related_website_sets.JSON") | ||
| else: | ||
| temp_sets.update({testing_set: check_sets[testing_set]}) |
There was a problem hiding this comment.
FYI, using .update is overkill here:
| temp_sets.update({testing_set: check_sets[testing_set]}) | |
| temp_sets[testing_set] = check_sets[testing_set] |
check_sites.py
Outdated
| if testing_sets: | ||
| temp_sets = {} | ||
| for testing_set in testing_sets: | ||
| if testing_set not in check_sets: | ||
| error_texts.append("There was an error loading the set:\n" + testing_set + | ||
| " could not be found in related_website_sets.JSON") | ||
| else: | ||
| temp_sets.update({testing_set: check_sets[testing_set]}) | ||
| check_sets = temp_sets |
There was a problem hiding this comment.
Personal preference, but I'd lean toward writing this in two steps: first validate the arguments, then compute the subset of the check_sets dict. E.g.:
| if testing_sets: | |
| temp_sets = {} | |
| for testing_set in testing_sets: | |
| if testing_set not in check_sets: | |
| error_texts.append("There was an error loading the set:\n" + testing_set + | |
| " could not be found in related_website_sets.JSON") | |
| else: | |
| temp_sets.update({testing_set: check_sets[testing_set]}) | |
| check_sets = temp_sets | |
| if testing_sets: | |
| absent_primaries = [p for p in testing_sets if p not in check_sets] | |
| if absent_primaries: | |
| # print some error and quit | |
| check_sets = {p: check_sets[p] for p in testing_sets} |
check_sites.py
Outdated
| for opt, arg in opts: | ||
| if opt == '-i': | ||
| input_file = arg | ||
| if opt == '--data_directory': | ||
| input_prefix = arg | ||
| if opt == '--with_diff': | ||
| with_diff = True | ||
| if opt == '--testing_sets': | ||
| testing_sets = arg.split(',') |
There was a problem hiding this comment.
| testing_sets = arg.split(',') | |
| testing_sets.extend(arg.split(',')) |
Just in case people supply the arg multiple times.
|
Looks like you've passed all of the checks! |
* Add fix for local testing * Change implementation so a list of primaries can be passed * Make suggested fixes to local testing command * reformat messaging * Make examples more explicit
Currently, if users would like to locally test their changes to related_website_sets.JSON, they have to use an awkward workaround. This change makes the workflow much simpler for users by adding an optional command line argument they can pass to check_sites.py when running it locally.