Nuclide run-through #2

[vjeux]

Since the first one went really well, let's do another round :)

The issues are getting more scoped and less serious :)

• Should put the arrow function body on the next line instead of breaking it in weird ways

• I think I like the arrow function better for type

• Would be nice to make empty body have the } on their own line

• Space before would be nice

• Wrong code generated because of comment placement

• Invalid code generated because of trailing comma

• Would be nice to keep the comment where it was

• If one && is broken, would be nice to break all of them into their own line

• Missing type annotation for argument spread

• Missing ; for export default

• Missing parenthesis for ++

• Missing generic

• Import doesn't break

[jlongster]

Thanks! Yeah, comments are pretty buggy right now. I removed a bunch of logic from recast because I need to convert it to my new API, so still need to do that. I think it's very difficult to keep comments exactly where they are, but we'll see what we can do.

I'm going to focus mostly on your first run-through before announcing it next week. It would be great if others began picking up some of the smaller tweaks and opened PRs after that. It may be good to break this one up into individual issues so that's easier.

[jlongster]

Should put the arrow function body on the next line instead of breaking it in weird ways

This was because of the last arg grouping. It's an anon function so it lets it break itself. However I think it makes sense to only allow that if it's an arrow function with braces. I fixed this in 892d070

[jlongster]

I think I like the arrow function better for type

I'd like more feedback from Flow users... I think I got this logic from recast. I believe it's easy to tweak if we do want this.

[jlongster]

Would be nice to make empty body have the } on their own line

I like the empty block :) These are small diffs, do you think little diffs like that will make it a lot harder for you to pitch this?

For things like like this I would like more feedback from the community, but I also want to prioritize helping make this work for you. I think getting it work on nuclide is a strong use case and I'm fine tweaking the defaults to make that work.

[jlongster]

Invalid code generated because of trailing comma

Looks like this was already fixed by another commit

[vjeux]

I like the empty block :)

Cool, that's good. There aren't many empty blocks in real world anyway :p

[jlongster]

Cool, that's good. There aren't many empty blocks in real world anyway :p

I may have just convinced myself that maybe it's better to split them across 2 lines because then it's easier to add code to them... but this can easily be changed later.

[slobo]

Cool, that's good. There aren't many empty blocks in real world anyway :p

I may have just convinced myself that maybe it's better to split them across 2 lines because then it's easier to add code to them... but this can easily be changed later.

} catch (err) {}

should format to

} catch (and_ignore_err) {}

only half joking...

[karl]

Invalid code generated because of trailing comma

Looks like this was already fixed by another commit

Do you have a link to the commit that fixes this? I'm still seeing it on version 0.0.4. This code:

const LongVariableName = longMethodName(longParamName, longSecondParamName, () => {});

becomes:

const LongVariableName = longMethodName(longParamName, longSecondParamName, (
  ,
) => 
  {});

Which is invalid as the comma is inside the empty parameter list.

https://vjeux.github.io/prettier-browser/#%7B%22content%22%3A%22const%20LongVariableName%20%3D%20longMethodName(longParamName%2C%20longSecondParamName%2C%20()%20%3D%3E%20%7B%7D)%3B%5Cn%22%2C%22options%22%3A%7B%22printWidth%22%3A80%2C%22tabWidth%22%3A2%2C%22useFlowParser%22%3Afalse%2C%22singleQuote%22%3Afalse%2C%22trailingComma%22%3Atrue%2C%22bracketSpacing%22%3Afalse%7D%7D

[jlongster]

Yeah, there's another bug about that too. I'll look into it.

[vjeux]

Just sent #133 and #134 for the types that didn't print on babylon parser.

[vjeux]

Closing this one, most of those are either done or already have open issues!