-
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
Improve validation error messages #5
Conversation
32f7d2f
to
dbb3cd4
Compare
This PR now includes updating the schema to the 0.1 version currently on master. There's also a fixture file to display all the errors: https://github.com/openownership/lib-cove-bods/pull/5/files#diff-8a78820a9db4bf7876c2083f107362f9 |
b01c917
to
59d8551
Compare
I've just seen this - sorry, I should have focussed on getting this merged in when the sprint started. However, now OpenDataServices/lib-cove has been released can we use a proper tag here? Can this be changed? In general, I think we should not be switching from a tag to a commit hash unless there are very unusual circumstances. It makes it much harder for a reader to trace what's going on, and opens up the possibility that we end up using unreleased versions of libraries from branches. The proper procedure should be to go and release the library, if needed. |
Also, can we update Changelog with the library change - like https://github.com/OpenDataServices/lib-cove/blob/master/CHANGELOG.md - and the change to schema? Thanks. |
No, because the pull request this is related to has not been merged https://github.com/OpenDataServices/lib-cove/pull/8/files (due to the impact on OCDS and 360Giving CoVE). |
4735dc9
to
e77daca
Compare
This reverts commit 8b2c0d6.
This PR now contains the error validation message text changes themselves, so that we can make this change for BODS independent of OCDS and 360Giving. More info openownership/cove-bods#16 (comment) |
Once OpenDataServices/lib-cove#14 is in a release, this PR just needs the requirements updating. |
OpenDataServices/lib-cove#14 is now released. |
Sorry .... is there anything we can do to avoid
We are trying to work towards taking Django out as a requirement for this library, and other libraries like it. In other libraries, it's a requirement for historical reasons, but for this library we don't have that, so can we avoid it? If it really is totally unavoidable, fair enough, but that's a shame. Our discussion on Loomio about "black boxing" the output of these libraries may be relevant here? |
It's possible, but a bit tricky, particularly since it depends on lib-cove's behaviour, which also uses the Django function. Also, I won't get chance to look at this this week. As mentioned on a call, I'd rather get this merged, so CoVE BODS can have this functionality, and work out what to do about removing Django deps from there. |
openownership/cove-bods#16
Depends on this lib-cove PR OpenDataServices/lib-cove#8Depends on this lib-cove PR OpenDataServices/lib-cove#14