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

Don't let Path inherit from dict #190

Merged
merged 1 commit into from Apr 4, 2018

Conversation

@lafrech
Copy link
Member

commented Mar 29, 2018

While investigating #189, I've been wondering why Path inherits from dict and in the meantime holds content in its operations attribute.

It looks like most content is duplicated in operations and in Path itself. In fact, inheriting from dict seems kinda strange.

This PR removes this inheritance to simplify the code. It makes more sense to me this way. It brings no enhancement, but tries to improve maintainability.

What was the reason for the original design? Any objection to this simplification?

I had to modify 2 tests to get this going. This could mean I'm totally mistaken, but from the name of the tests, I don't think the behaviour I modified is what was being tested, rather a side-effect.

Feedback welcome.


Note that if this PR is applied, we'll probably need to change

self.operations = operations or {}

into

self.operations = operations or OrderedDict

to get the behaviour proposed in #189.

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 30, 2018

Hm, I don't recall why I made Path a dict... I wonder if that decision predated the to_dict method and we relied on dict(path) for serializing Paths.

This PR is probably the right way to go.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

Great.

How shall we proceed? Should I merge this?

Considering the modifications in the tests, do you consider this a breaking change? I would say it isn't and those tests were wrongish, but maybe I'm focusing on my own experience of apispec.

@sloria

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

Yeah, I think this can be merged as is. It looks like a breaking change, but it shouldn't affect most users.

@lafrech Is this still WIP? If not, can you rewrite the commit message without "WIP"?

@lafrech lafrech force-pushed the Nobatek:dev_path_object branch from 1767a2b to 15c11d3 Apr 4, 2018

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2018

Alright, I rebased, reviewed and modified the commit message.

I checked those tests again and I don't think anyone should be affected.

Thanks.

@sloria sloria merged commit d08e3db into marshmallow-code:dev Apr 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lafrech lafrech deleted the Nobatek:dev_path_object branch Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.