-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an xmlvalidate command #34
base: main
Are you sure you want to change the base?
Conversation
70eb8c2
to
5605d5d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
=======================================
Coverage 81.88% 81.88%
=======================================
Files 16 16
Lines 436 436
=======================================
Hits 357 357
Misses 55 55
Partials 24 24 ☔ View full report in Codecov by Sentry. |
5605d5d
to
3d926ef
Compare
Add `xmlvalidate/cmd/xmlvalidate.go` to allow running xmlvalidate from the CLI for testing purposes.
3d926ef
to
3d650f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @djjuhasz, not sure if it adds a lot of value, but check my comments below if you find it useful.
XMLFILE Path of the XML file to be validated | ||
|
||
OPTIONS: | ||
--xsd PATH Path of an XSD file to validate against |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's required, I'd make it an argument too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it an option for two reasons:
- It avoids confusing the order of the XML file path and XSD file path, e.g.
xmlvalidate person.xsd people.xml
vs.xmlvalidate people.xml person.xsd
- It leaves open the option of adding a
--dtd
option to validate XML against a DTD file instead of an XSD
I think the first reason justifies typing the extra --xsd
.
} else { | ||
fmt.Println(out) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recent PRs changed the xmlvalidate
activity but didn't reflect those changes in the docs (parameter names, result type, how to use the XMLLintValidator
). It would be nice to include a note somewhere in there about this command too, if we really want to include it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! I'll update the docs. 😊
} | ||
|
||
// To test: `go run ./cmd --xsd testdata/person.xsd testdata/person_valid.xml` | ||
func main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to follow https://artefactual.dev/docs/programming-languages/go/main/ and add some tests for this command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, okay. 👍
Add an
xmlvalidate
command to help testing the functionality from the CLI.