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

adding meta ordered preserves order. #43

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stringfellow
Copy link
Contributor

@stringfellow stringfellow commented Sep 25, 2017

Property order wasn't preserved when the Meta field 'ordered' was set to True. 'tis now!

@coveralls
Copy link

coveralls commented Sep 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 0b9f45d on stringfellow:bugfix/restore-property-order into dde52eb on fuhrysteve:master.

@coveralls
Copy link

coveralls commented Sep 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 9e8683a on stringfellow:bugfix/restore-property-order into dde52eb on fuhrysteve:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9e8683a on stringfellow:bugfix/restore-property-order into dde52eb on fuhrysteve:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9e8683a on stringfellow:bugfix/restore-property-order into dde52eb on fuhrysteve:master.

@fuhrysteve
Copy link
Owner

Doesn't the order get thrown out when it hits the json encoder unless you use the object_pairs_hook flag? https://docs.python.org/3/library/json.html#json.JSONDecoder

@stringfellow
Copy link
Contributor Author

seemed to work ok with the stack I had with brutusin jsonforms - all came out in the right order

@stringfellow
Copy link
Contributor Author

(err, but this was python 2..)

@stringfellow
Copy link
Contributor Author

@fuhrysteve just looking at this again, I'm not immediately sure where to slot in the decoder... do you have any pointers/would you consider looking at it?

@stringfellow stringfellow mentioned this pull request Dec 13, 2017
@stringfellow
Copy link
Contributor Author

Hello! Sorry to bother again, I was just trying to switch to 0.5.0 and I realised I'm still using my own fork because of this outstanding PR - please could you help me out getting it to a state you're happy with for merge?

@atmo
Copy link
Contributor

atmo commented Jul 22, 2019

Hi @stringfellow ! I don't really understand what the use case for this feature is. Even though dumping OrderedDict to json seems to work and the resulting json will indeed preserve the order of fields in the original marshmallow schema, this solution seems very hacky. As there is no option in jsonschema spec to assert that order, it can (and will) be lost if, for example, some other Python program will read this schema from file.

I agree with @fuhrysteve that if you need to define some order for your fields, you'll have to use some kind of other UI schema for that. I recommend closing this PR.

@fuhrysteve
Copy link
Owner

@stringfellow For what it's worth, if you're using python 3.6 or above, all dicts are ordered anyways. Anything we can do to make it easier for you to move upstream without having to maintain your own fork?

@stringfellow
Copy link
Contributor Author

stringfellow commented Jul 24, 2019

Hi both, I'm grateful for the attention on this PR - thanks for taking the time to look at it.

To answer @atmo first, the (our) use-case is this:

  1. Author of a Marshmallow Schema specifies fields in some order, e.g. a set of fields to capture an address.
  2. Author of that schema wishes to use same schema in a front-end application where render order of fields matters.
  3. Author does not wish to "repeat themself" (following DRY principles) by writing out every single field in the front end as they have already done so (presumably in some well considered way) in the back end and the purpose of the glue library they are using (i.e. this) is to authentically replicate what they already spent time authoring in a different format.
  4. Author therefore hopes (but does not expect, of course) that the glue library will do everything it can to authentically replicate their original python code (even if that goes beyond the minimum it has to do), if it has no side-effects that harm any other user of the glue library.

Put simply - if one spends time writing a Python model carefully, and then the serialisation of it arbitrarily changes things because it is "allowed" to (but doesn't have to) - that is really annoying particularly when there is a simple fix that also does not break the specification (and users of that serialisation can assume it will perform to the letter of the spec and be delighted by it sugar-coating and assisting them as one would hope a good library written for time-constrained developers might).

And so, the "type" of ordering I applied to the collection is in contest because there is no ordering specified (and indeed, an object is "unordered") in the specification - true - however I would just say that applying any ordering here would fall to a similar argument e.g. sorted() here- https://github.com/fuhrysteve/marshmallow-jsonschema/blob/master/marshmallow_jsonschema/base.py#L109 and this little assistance has a profound reduction in work for a front-end developer who consumes the output of this library.

I contest that my solution is simply closer to what the original (Python) author of a schema in Marshmallow might want to see or provide to a front-end developer - i.e. the fields in the order they described them in their python model, even though it is true that ordering cannot be guaranteed - and you are absolutely right that this cannot be guaranteed and nor should a consumer of the resulting JSON in any other language assume it will - however there are a subset of cases (as mine) for which this is inordinately useful. I would say "hacky" is a touch unfair too ;)

@fuhrysteve -- thanks for your offer and the info - we've gradually moving toward python 3.7 ahead of deprecation of python 2 so hopefully we soon won't need this fork! If my argument above sways you at all then I'd be very happy to help get this through to master ASAP - I didn't quite understand your comment in sept 2017 and have not had any time or headspace to dig into this unfortunately - though we have been using my fork in production for ~2 years and I can offer good evidence that it does exactly what I wanted (I have not had a form field ordering other than which I specified in the Python model since switching to this fork).

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.

4 participants