Skip to content

Validation errors#525

Merged
calina-c merged 11 commits intomainfrom
validation-errors
Sep 7, 2022
Merged

Validation errors#525
calina-c merged 11 commits intomainfrom
validation-errors

Conversation

@calina-c
Copy link
Copy Markdown
Contributor

Closes #523

@calina-c calina-c self-assigned this Aug 10, 2022
@calina-c calina-c marked this pull request as ready for review August 11, 2022 04:31
@calina-c calina-c requested a review from alexcos20 August 11, 2022 04:31
@calina-c
Copy link
Copy Markdown
Contributor Author

I'm now thinking about another possible improvement: the error returned can be a key => value mapping:

  • the key would be the field it arised on, nested with dots
  • the value would be the error itself

@bogdanfazakas @alexcos20 would you like that? I'd like this intermediary version reviewed, tested and merged though, since it contains structural changes as well.

Comment thread ocean_provider/validation/algo.py Outdated
@alexcos20
Copy link
Copy Markdown
Member

I support key => value errors descriptions, it makes it more readable by devs

Comment thread ocean_provider/validation/algo.py Outdated
Comment thread ocean_provider/validation/algo.py Outdated
@bogdanfazakas
Copy link
Copy Markdown
Member

I support key => value errors descriptions, it makes it more readable by devs

I agree with Alex here, so besides the improvements from this PR would be nice to have key => value errors descriptions and use the new strings as values and some error codes as keys

@calina-c
Copy link
Copy Markdown
Contributor Author

@bogdanfazakas is it ok with you if we merge this, since it contains structural changes? Then add in the key value logic?

@bogdanfazakas
Copy link
Copy Markdown
Member

@bogdanfazakas is it ok with you if we merge this, since it contains structural changes? Then add in the key value logic?

The current state does not make our life easier on the client side, what about the previous suggestions from the PR ?, I'm fine with adding the key value logic later but, let's discuss the other stuff before merging.

Copy link
Copy Markdown
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, small issues that we can handle later
@bogdanfazakas - any last minute comments?

@bogdanfazakas
Copy link
Copy Markdown
Member

lgtm, small issues that we can handle later @bogdanfazakas - any last minute comments?

lg, I think we can handle the changes in the market, does this really close #523 also ?

@calina-c
Copy link
Copy Markdown
Contributor Author

calina-c commented Sep 7, 2022

lg, I think we can handle the changes in the market, does this really close #523 also ?

yep, all error messages are now key-value pairs with status messages :)

@calina-c calina-c merged commit d1556bb into main Sep 7, 2022
@calina-c calina-c deleted the validation-errors branch September 7, 2022 12:12
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.

Add a status code on C2D errors

3 participants