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

Remove space config #29

Merged
merged 30 commits into from
Oct 12, 2017
Merged

Conversation

gingermusketeer
Copy link
Member

This removes most of the space related configuration as part of #2. From the current discussions in #2 the changes being made here appear to have agreement on the behaviour.

The remaining configuration related to spacing had some discussion around it so I think it would be best to address that in a separate PR.

This is quite a big diff :( let me know if I should break this up!

Copy link
Member

@bessey bessey left a comment

Choose a reason for hiding this comment

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

Fantastic! I reviewed every spec and didn't find a single example out of line with #2.

@bessey
Copy link
Member

bessey commented Oct 12, 2017

One thought... I'm not sure what constitutes a breaking change for a formatter, but I imagine suddenly ignoring parts of someone's custom config is one of them, so we should at very least document that in the Changelog.

Perhaps we should even detect the no longer supported options, and warn the user at runtime? Not sure how one can warn other than STDERR + bad exit code though, which seems a bit ugly for those running via an editor extension.

Copy link
Member

@splattael splattael left a comment

Choose a reason for hiding this comment

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

@gingermusketeer Wow, well done 👍 Thank you 😃

@splattael
Copy link
Member

@gingermusketeer One thing. Do you mind to update https://github.com/ruby-formatter/rufo/wiki/Settings?

@splattael splattael added this to the Release v0.2.0 milestone Oct 12, 2017
@gingermusketeer
Copy link
Member Author

@splattael It does not look like it is possible to create a PR against the wiki repo and I don't have permission to update the wiki pages. I could create a gist with the new content if that works?

@bessey Good catch, I think detecting invalid config is a great idea, definitely help prevent spelling mistakes causing frustration as well as changes to rufo. I will update the change log when I get a moment.

Copy link
Member

@mjago mjago left a comment

Choose a reason for hiding this comment

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

consume_one_dynamic_space() can lose it's argument

consume_one_dynamic_space_or_newline() can also lose it's argument

@mjago
Copy link
Member

mjago commented Oct 12, 2017

Changes are great - thanks @gingermusketeer ! I think the invalid config detection is a separate PR. I'm undecided whether a typo should cause "no formatting" and an error rather than good formatting and no error?

@lucerion
Copy link
Member

@gingermusketeer good job! Thank you!

@splattael
Copy link
Member

@splattael It does not look like it is possible to create a PR against the wiki repo and I don't have permission to update the wiki pages. I could create a gist with the new content if that works?

@gingermusketeer Oh, right 😳 I've just invited you as a collaborator 😁

@gingermusketeer
Copy link
Member Author

@splattael Awesome!! Thanks a lot. I have really enjoyed working on this project so far :)

@gingermusketeer
Copy link
Member Author

@mjago Thanks for spotting that. I have merged in the changes from master and cleaned that up.

@mjago mjago merged commit a0e5453 into ruby-formatter:master Oct 12, 2017
@mjago
Copy link
Member

mjago commented Oct 12, 2017

Thanks @gingermusketeer ! I formatted the ruby spec corpus with the changes and all looks good from what I have seen. And fast 👍

@gingermusketeer gingermusketeer deleted the remove-space-config branch October 12, 2017 18:39
@gingermusketeer
Copy link
Member Author

@splattael I have updated the settings page by removing the old config options that were described there.

@splattael
Copy link
Member

@gingermusketeer Thank you! 💚

I've added a warning about the future removal of the remaining settings at https://github.com/ruby-formatter/rufo/wiki/Settings#warning-settings-are-going-away-warning.

Feel free to correct my English 😶

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.

6 participants