search

openownership / lib-cove-bods

Check that your data complies with the Beneficial Ownership Data Standard (BODS) using our install our data review library to analyse files via your command line interface
https://datareview.openownership.org/
Other
1 stars 0 forks source link

Improve validation error messages #5

Closed Bjwebb closed 5 years ago

Bjwebb commented 5 years ago

openownership/cove-bods#16

Depends on this lib-cove PR https://github.com/OpenDataServices/lib-cove/pull/8 Depends on this lib-cove PR https://github.com/OpenDataServices/lib-cove/pull/14

Bjwebb commented 5 years ago

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 We probably want to add a test that the output from that is what we expect, based on the recent changes to lib-cove. We will also need to update the OCDS/360Giving tests in the cove repo.

odscjames commented 5 years ago

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.

odscjames commented 5 years ago

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.

Bjwebb commented 5 years ago

However, now OpenDataServices/lib-cove has been released can we use a proper tag here? Can this be changed?

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).

Bjwebb commented 5 years ago

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 https://github.com/openownership/cove-bods/issues/16#issuecomment-478426310

Bjwebb commented 5 years ago

Once OpenDataServices/lib-cove#14 is in a release, this PR just needs the requirements updating.

odscjames commented 5 years ago

OpenDataServices/lib-cove#14 is now released.

odscjames commented 5 years ago

Sorry .... is there anything we can do to avoid 

 from django.utils.html import format_html

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?

Bjwebb commented 5 years ago

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.