Validation CLI #508
Validation CLI #508
Conversation
owencjones
left a comment
owencjones
left a comment
There was a problem hiding this comment.
Looks good, no required changes, except the double backticks for the Sphinx docs, the rest are just me being picky 😉
| _error(str(e)) | ||
| except UnrecognizableFileEncoding as e: | ||
| _error(str(e)) | ||
| except UnknownAnnotationFileSchema as e: |
darwin/cli_functions.py
Outdated
| except NotFound as e: | ||
| _error(f"No dataset with name '{e.name}'") | ||
| except: | ||
| _error(f"An error has occurred, please try again later.") |
There was a problem hiding this comment.
Not your code, but f-string with no placeholders, could remove.
darwin/cli_functions.py
Outdated
| filename for saving to output, by default None | ||
| """ | ||
| if not files and not folder and not pattern: | ||
| # TODO insert logging here once we introduce that |
There was a problem hiding this comment.
In the meantime, this is a CLI command, so move L974 above this and
| # TODO insert logging here once we introduce that | |
| console.log('--files or --folder flag requires a pattern') |
to give the user firm output on their issue?
https://rich.readthedocs.io/en/stable/console.html#attributes - Rich has a lot of stuff for this, and it can be handy.
There was a problem hiding this comment.
I'm also pretty sure that the argparse parsing actually makes this condition impossible, but handling it is good to avoid someone breaking it in future.
darwin/cli_functions.py
Outdated
| except MissingSchema as e: | ||
| errors = [e] | ||
| all_errors[str(file)] = [str(error) for error in errors] | ||
| if len(errors) == 0: |
There was a problem hiding this comment.
Same as putting
if not errors:
darwin/exceptions.py
Outdated
| Parameters | ||
| ---------- | ||
| parent_error: ValidationError | ||
| Error reported by `jsonschema`. |
There was a problem hiding this comment.
For Sphinx docs, we need to use double backticks.
Co-authored-by: Owen Jones <owencjones+github@gmail.com>
rslota
left a comment
rslota
left a comment
There was a problem hiding this comment.
I'm not a fun of the --file / --folder flags tbh. It's not consistent with our other commands in CLI which detect automatically if user provided a file or a folder.
Ideally users could just do the same as they do with import, but just changing import to validate in the shell and get validation, without looking at other things they need to provide for commend to work. It feels like making user decide between file and folder is just worse UX, as it's not like the argument is ambiguous. As long it's not a pattern, we can detect if it's a folder or file. Pattern could stay more complex case, for those wanting to use it.
What do you think @Nathanjp91 @owencjones ?
Let's also amend the PR title to include ticket code and [extenal] tag please 🙏
Also, the validation doesn't feel verbose enough. For example, I made a typo in one of the fields in my 100k line JSON and I got:
As user, I would have no clue which name this is referring to. OpenAPI is capable printing very good location of the errors and we really should include that, otherwise it's pretty hard to know which of 20k annotations contains the bug.
Exposes a new CLI function
darwin validate locationwhere location is a single file or a folder to recursively search for *.json files
glob style pattern based search
--pattern PATTERNwill print to the console each file checked and pass/fail + list of errors found on failed files
optional flags to turn off console printing for successful files
--silentand a flag to specify an output file to save the result
--output OUTPUTChanges to code base:
jsonschema had to be updated to support the draft202012Validator used for darwin v2 schema which was introduced in a later version than what the repo was based on. This unfortunately changes a range of tests and needed to be updated to keep in line with the Exception changes