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

[RFC] Keep expanded objects expanded #495

Merged
merged 1 commit into from Jan 30, 2017

Conversation

vjeux
Copy link
Contributor

@vjeux vjeux commented Jan 27, 2017

Another attempt at solving the issue where objects are not expanded the way people expect. If there's any new line in the original source, it's going to expand it. This gives more control to the user in how the objects should be formatted.

Fixes #74

@@ -73,6 +73,8 @@ function utf16Length(str, pos) {
exports[`test keys.js 1`] = `
"({\'この事はつもり素晴らしいことさ\': \'35jL9V\'})
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
({ \"この事はつもり素晴らしいことさ\": \"35jL9V\" });
({
\"この事はつもり素晴らしいことさ\": \"35jL9V\"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bug in the flow parser, it incorrectly computes the length of the string with unicode characters inside.

@jlongster
Copy link
Member

A lot of the tests changes are good example why I'm hesitant to do this. We should help the user simplify the code and a ton of those changes look like regressions to me. This seems like a place ripe for disagreements on a team and "nits" to start coming through to tell them to put an object on a single line because it's so small and fits inside the function call, etc.

This might be one of those things where I'd rather wait a while before doing, because we can't remove it after landing. I'd like to hear more from users about if this is an actual problem (only heard from a few that do, but many others may not want it)

@vjeux
Copy link
Contributor Author

vjeux commented Jan 27, 2017

Yeah, I'm not really sure about it either. I'll keep sending various ideas to fix the problem :)

@jlongster
Copy link
Member

Some of them seem like positive changes though:

screen shot 2017-01-27 at 1 58 38 pm

I suppose it does allow for consistency, so you might be onto something. Let's ask for more feedback about it.

@gaearon
Copy link

gaearon commented Jan 27, 2017

This looks much cleaner to me. When this kind of code evolves people end up adding more properties and you expand anyway at some point. Might as well do this right away. For types, you can always hoist them up for readability.

@rattrayalex
Copy link
Collaborator

Hmm this one feels a little concerning to me... Agreed it may be worth sitting on a bit...

@vjeux
Copy link
Contributor Author

vjeux commented Jan 28, 2017

Some random thoughts on the subject:

The first attempts at solving the nits/formatting problem look like eslint --fix, where you would first look for bad patterns and then make them better automatically. The great thing is that if the user isn't breaking the rules, he can keep the formatting s/he wants. However, in practice this is very hard to implement properly outside of particular cases.

The second iteration is something like refmt where you reformat all the code with no exceptions. This is great and consistent but it is very inflexible. Developers want some control over how the code is formatted. #7 is an example that exploits the fact that empty lines are added before comments, so the person puts some random comments in order to make the code breathe.

I think that in order to succeed we need a middle ground. Where most of the code is automatically formatted but the user still has some control for cases where it cannot be always be automatically formatted in a reasonable way. We've done this successfully so far for empty lines, and objects may be a place where this would work as well.

@vjeux
Copy link
Contributor Author

vjeux commented Jan 28, 2017

And, if we remove all the nits except for ones on objects, I still think the project would be super successful :)

@jlongster
Copy link
Member

And, if we remove all the nits except for ones on objects, I still think the project would be super successful :)

Alright. I don't have strong enough objections to hold this back. I'm fine moving forward with it. I don't want this to open the doors for other customizations based on input code; we should really try to restrict it. But I don't see this doing a lot of harm.

@vjeux
Copy link
Contributor Author

vjeux commented Jan 30, 2017

Let's try this and revert it if it leads to people abusing it :)

Another attempt at solving the issue where objects are not expanded the way people expect. If there's any new line in the original source, it's going to expand it. This gives more control to the user in how the objects should be formatted.

Fixes prettier#74
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants