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

config: alphabetize feature flags #3309

Merged
merged 1 commit into from
Feb 15, 2016
Merged

Conversation

rralian
Copy link
Contributor

@rralian rralian commented Feb 15, 2016

This is a lower-priority cleanup PR. There are a ton of them, and they're not in any sensible order. That makes them harder to find and would make it pretty easy to accidentally set a feature in two spots.

I did this by copying the feature object into a javascript console.

set features

And then copying the output of that variable (because the console auto-alphabetizes attributes)

alphabetized in console

And then I copied those lines into an editor and did a regex search/replace to get them into a usable format (quotes around attribute name and comma at the end).

replace: ^([^:]+)([^\n]+) with "$1"$2,

Testing

I realize this is a difficult PR to review visually, because it would be easy to miss an omission or change in the diff. Probably the best way to make sure I didn't do something crazy is to count the lines removed and added for each file (keeping in mind I am also removing blank lines). Also ensure the final output file looks correct for each features object, with all attribute names quoted, commas at the end of each line except the last attribute, and feature names alphabetized.

@rralian rralian added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Feb 15, 2016
@rralian rralian self-assigned this Feb 15, 2016
@rralian rralian added this to the Calypso Core: Offline 4 milestone Feb 15, 2016
@nylen
Copy link
Contributor

nylen commented Feb 15, 2016

I like this 👍

I wrote a quick script to test it, and everything looks good: https://gist.github.com/nylen/51f212025af7e7b76953

OK: horizon.json
OK: wpcalypso.json
OK: desktop.json
OK: development.json
OK: production.json
OK: stage.json
OK: desktop-mac-app-store.json

As a bonus of having an automated test, if you want to wait until after #876 lands, you can re-do the test pretty easily.

@nylen nylen added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 15, 2016
@nylen
Copy link
Contributor

nylen commented Feb 15, 2016

Also - might be a good idea to add a note to config/README.md to keep the config keys sorted when adding new items.

@rralian
Copy link
Contributor Author

rralian commented Feb 15, 2016

Thanks @nylen, good point on the docs... added.

@rralian
Copy link
Contributor Author

rralian commented Feb 15, 2016

rebasing and will run that test again... thanks for making that, btw.

There are a ton of them, and they're not in any sensible order. That will makes them harder to find and would make it pretty easy to accidentally set a feature in two spots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants