Skip to content
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

Merged
merged 12 commits into from
May 31, 2019
Merged

Conversation

Bjwebb
Copy link
Collaborator

@Bjwebb Bjwebb commented Feb 5, 2019

openownership/cove-bods#16

Depends on this lib-cove PR OpenDataServices/lib-cove#8
Depends on this lib-cove PR OpenDataServices/lib-cove#14

rhiaro
rhiaro previously approved these changes Feb 6, 2019
@Bjwebb Bjwebb force-pushed the cove-bods-16-oneof-validation branch 3 times, most recently from 32f7d2f to dbb3cd4 Compare February 18, 2019 06:20
@Bjwebb Bjwebb changed the title Add tests of improved validation error messages for oneOf Add tests of improved validation error messages Feb 18, 2019
Bjwebb added a commit to openownership/cove-bods that referenced this pull request Feb 18, 2019
@Bjwebb
Copy link
Collaborator Author

Bjwebb commented Feb 19, 2019

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.

rhiaro
rhiaro previously approved these changes Feb 20, 2019
@odscjames
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

Bjwebb commented Mar 21, 2019

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

@odscjames odscjames changed the title Add tests of improved validation error messages WIP: Add tests of improved validation error messages Mar 21, 2019
@Bjwebb Bjwebb force-pushed the cove-bods-16-oneof-validation branch 3 times, most recently from 4735dc9 to e77daca Compare March 22, 2019 08:49
@Bjwebb Bjwebb changed the title WIP: Add tests of improved validation error messages WIP: Improve validation error messages Apr 1, 2019
@Bjwebb
Copy link
Collaborator Author

Bjwebb commented Apr 1, 2019

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)

@Bjwebb
Copy link
Collaborator Author

Bjwebb commented Apr 2, 2019

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

@odscjames
Copy link
Collaborator

OpenDataServices/lib-cove#14 is now released.

@Bjwebb Bjwebb requested review from rhiaro and odscjames April 29, 2019 07:41
@Bjwebb Bjwebb changed the title WIP: Improve validation error messages Improve validation error messages May 6, 2019
@odscjames
Copy link
Collaborator

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
Copy link
Collaborator Author

Bjwebb commented May 30, 2019

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.

@odscjames odscjames merged commit cc6f265 into master May 31, 2019
@odscjames odscjames deleted the cove-bods-16-oneof-validation branch May 31, 2019 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants