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

Sort Properties on Final Netkan Output #1304

Merged
merged 4 commits into from
Aug 21, 2015
Merged

Conversation

dbent
Copy link
Member

@dbent dbent commented Jul 16, 2015

The order of properties in the final .ckan output of Netkan is currently dependent on the order in which code is executed by the various transformers. This PR adds a new ITransformer, PropertySortTransformer that is added to the very end of the pipeline.

PropertySortTransformer has an internal list of known properties and will sort the properties in a well defined order (unknown properties are placed just before the x_generated_by property in alphabetical order, with x_ properties always sorting last).

This means the output of Netkan should remain relatively stable with all sorts of changes.

Sort order was chosen based on what I feel is an intuitive reading order when looking at .ckan files, and mirrors the typical order found in handcrafted .ckan files.

@mheguy
Copy link
Contributor

mheguy commented Jul 16, 2015

Thanks for changing the order. =)

@techman83
Copy link
Member

Just be prepared for a netkan commit deluge when the changes go through!

@dbent
Copy link
Member Author

dbent commented Jul 20, 2015

Just be prepared for a netkan commit deluge when the changes go through!

True story, but then after this future changes will be less likely to produce such mass updates, woo.

@Dazpoet
Copy link
Member

Dazpoet commented Aug 20, 2015

Am I reading this correctly that all comments will now be placed at the top (line 2) in the .ckan files? What about examples like this where the comments placement is related to its information?

@mheguy
Copy link
Contributor

mheguy commented Aug 20, 2015

Nested comments remain nested (ex. comments in resources or install stay inside).

And yeah I'm misusing the term nested, but you get what I mean.

@Dazpoet
Copy link
Member

Dazpoet commented Aug 20, 2015

Ah makes sense :) I'll admit that my codereading skills are <=0 but this looks good to me and might finally make things formatted in similar ways which would make my ocd very happy

@dbent dbent force-pushed the topic/property_sort_transformer branch from 23dbb4a to ff4a36f Compare August 20, 2015 13:19
Dazpoet added a commit to KSP-CKAN/CKAN-meta that referenced this pull request Aug 21, 2015
Apart from making the filename match our given convention this also prepares this file for KSP-CKAN/CKAN#1304 which has a dislike for multiple non-nested comments.
@Dazpoet
Copy link
Member

Dazpoet commented Aug 21, 2015

Did some testing with some extremely malformed versions of our indexed CommunityResourcePack.netkan file and everything seems to work as it should. Having multiple non-nested versions of the same field in the same file leads to only the one closest to the bottom being listed in the created .ckan. I assumed comments to be the only field where such a thing could possibly be and only found three such cases and one is in a handwritten .ckan which is fixed by KSP-CKAN/CKAN-meta#744

The other two are in CacEye2.netkan and KSIPlacementServices.netkan neither of which should present a problem since the comments are only relevant for the .netkan files either way.

I don't speak code so I'll just trust Travis on that one.

To me the above thing with only keeping the bottom most one of non-nested duplicate fields isn't a showstopper so I'd say this is ready for a merge, however I'll let the clever netkan people know before merging and potentially blowing up ckan-meta 😈

@dbent dbent force-pushed the topic/property_sort_transformer branch from ff4a36f to ca41c4b Compare August 21, 2015 13:20
@Dazpoet
Copy link
Member

Dazpoet commented Aug 21, 2015

Discussed on irc and the above is not a showstopper so I'm merging this. @techman83 might see a gigantic thing pushed by netkan-bot as an effect if I understand the discussion in this PR correctly.

Dazpoet added a commit that referenced this pull request Aug 21, 2015
@Dazpoet Dazpoet merged commit 766650d into master Aug 21, 2015
@Dazpoet Dazpoet deleted the topic/property_sort_transformer branch August 21, 2015 13:29
@techman83
Copy link
Member

@Dazpoet hahaha, yep! ALL THE COMMITS!!

@techman83
Copy link
Member

And an excerpt from the bot log -> https://gist.github.com/techman83/7cda2f560b4d6cd864b5

@pjf
Copy link
Member

pjf commented Aug 24, 2015

@techman83 : It looks like the bot is very committed!

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.

5 participants