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

Declare json-patch operations as a class-based attribute #118

Merged
merged 3 commits into from
Dec 1, 2020

Conversation

tzoiker
Copy link

@tzoiker tzoiker commented Nov 23, 2020

I propose to declare available JSON-patch operations as class-based attributes rather than in the constructor.

Motivation:

  • It seems a bit more logical.
  • It became impossible to add custom operations without complete copy-paste of the constructor due to added validation.

Python 2.7 does not support MappingProxyType, so it should be handled with more care (due to dict mutability), although I suppose it is not much of a problem as python2.7 was deprecated.

@coveralls
Copy link

coveralls commented Nov 23, 2020

Coverage Status

Coverage decreased (-0.1%) to 65.846% when pulling 9310d48 on tzoiker:fix/json-patch-ops into f3c46e8 on stefankoegl:master.

@tzoiker tzoiker changed the title Declare json-patch operations as class-based attribute Declare json-patch operations as a class-based attribute Nov 24, 2020
Copy link
Owner

@stefankoegl stefankoegl left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! At first glance it looks like a worthwhile change. However I noticed two issues:

  • the tests fail for most python versions
  • the diff contains a lot of unrelated changes which should be removed (possible whitespace-only changes)

Please update your pull request accordingly. I'll be able to do a more detailed review afterwards.

Thanks!

@tzoiker
Copy link
Author

tzoiker commented Nov 24, 2020

  • the tests fail for most python versions

Fixed.

  • the diff contains a lot of unrelated changes which should be removed (possible whitespace-only changes)

I had to reorder JSONPatch and operations, otherwise, they are not found when JSONPatch is declared

@stefankoegl stefankoegl merged commit 57e4273 into stefankoegl:master Dec 1, 2020
@stefankoegl
Copy link
Owner

I just released version 1.28 with this change - thanks for your contribution!

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.

None yet

3 participants