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

Add Rule 127 - Use JSON as payload data interchange format #1225

Closed
wants to merge 1 commit into from

Conversation

vadeg
Copy link
Contributor

@vadeg vadeg commented Apr 12, 2021

Implement Rule 167

  • Add support for the acceptance list of media types

Fixes #1217

@tkrop
Copy link
Member

tkrop commented Apr 14, 2021

I'm not sure, but may be this implementation is going to far, since we explicitly relax this rule in #168.

@vadeg
Copy link
Contributor Author

vadeg commented Apr 14, 2021

@tkrop thanks for the comment. My impression was that in these cases API designers have to exclude Rule 167 explicitly. From API perspective it is hard to distinguish where should be JSON and where something else.

@tkrop
Copy link
Member

tkrop commented Apr 14, 2021

@vadeg Yes, I agree. Excluding rules is definitely an option. However, currently we do not encourage the usage of x-zally-ignore to prevent users from freely applying it to other rules too. I think we should carefully consider the impact of such a strategy change.

@vadeg vadeg added the question label Apr 14, 2021
@vadeg
Copy link
Contributor Author

vadeg commented Apr 16, 2021

Let's discuss this on our meeting.

@vadeg
Copy link
Contributor Author

vadeg commented May 6, 2021

We decided not to implement this rule, because this would lead many exceptions and it would be difficult to validate each case.

@vadeg vadeg closed this May 6, 2021
@vadeg vadeg reopened this May 19, 2021
@tkrop
Copy link
Member

tkrop commented May 20, 2021

May be you can separate 38da19a, so that we can merge this quickly.

@tkrop
Copy link
Member

tkrop commented May 20, 2021

The build seems to be failing because of the same reason as in #1227:

--- FAIL: TestIntegrationWithNoMustViolations (0.36s)
    integration_test.go:84: 
        	Error Trace:	integration_test.go:84
        	Error:      	Should be zero, but was 17
        	Test:       	TestIntegrationWithNoMustViolations
        	Messages:   	No MUST violations expected
    integration_test.go:88: 
        	Error Trace:	integration_test.go:88
        	Error:      	Expected nil, but got: &domain.AppError{e:(*errors.errorString)(0xc000182c00), c:3}
        	Test:       	TestIntegrationWithNoMustViolations

@vadeg
Copy link
Contributor Author

vadeg commented May 20, 2021

May be you can separate 38da19a, so that we can merge this quickly.

@tkrop this has been merged in #1222 . I will update this PR to remove this change.

@vadeg vadeg force-pushed the gh-1217-use-json-as-payload branch from 085a25c to fac1cce Compare May 21, 2021 09:49
@tkrop tkrop mentioned this pull request May 27, 2021
@tkrop tkrop assigned fmueller and vadeg and unassigned fmueller Jun 11, 2021
@tfrauenstein
Copy link
Member

hint: we discussed / decided the topic in the API guild meeting --> see https://docs.google.com/document/d/1NahSuMC0LRwFTTrln2m0iDLq0nHzGmF59Gv7xBhKPfQ/edit#bookmark=id.p63ihhuscg86

@ePaul
Copy link
Member

ePaul commented Sep 7, 2021

Instead of this, we want #1316.

@ePaul ePaul closed this Sep 7, 2021
@tkrop tkrop deleted the gh-1217-use-json-as-payload branch January 3, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editorial updates around Standard Data Format rules
5 participants