Conversation
|
What's needed to get this PR merged? This feature would be great to have! |
|
Hi! I apologise for how long it's taken us to look at this. It looks like a really valuable feature, and as I'm doing some work on this package towards a v2 release, I'd love to get this in as well. That said, I have some concerns about the code as-is. @clintwood, would you have the bandwidth to address some feedback here? |
motiz88
left a comment
There was a problem hiding this comment.
I think this mode also needs to support source maps, since the comment start/end characters will necessarily cause code to move around. I don't suppose this just happens to work if we pass --sourcemaps? 🙂 At any rate, the tests should cover this scenario.
At minimum, we should detect unsupported combinations of flags and output a clear error message.
| } | ||
| } else { | ||
| var toCommentLines = toComment.split(LINE_RX); | ||
| // TODO: detect file line endings scheme (\n or \r\n) |
There was a problem hiding this comment.
We should probably resolve this TODO. I wouldn't want to merge this feature if it messes up Windows line endings.
| toCommentLines.push('\n*/'); | ||
| result += toCommentLines.join(''); | ||
| } | ||
| } else if (!pretty) { |
There was a problem hiding this comment.
What happens if both --pretty and --comment are specified?
| DIFF=$(./flow-remove-types --pretty --sourcemaps inline test/source.js | diff test/expected-pretty-inlinemap.js -); | ||
| if [ -n "$DIFF" ]; then echo "$DIFF"; exit 1; fi; | ||
|
|
||
| # Test expected output with --comment option |
There was a problem hiding this comment.
We should probably also add the new test cases to test-update.sh.
|
With the greatest of respect I have switched to Typescript and do not have time to dig into this any longer. Please feel free to close or fork my branch and make the above changes. Thanks. |
This PR adds a
-C,--commentoptions to Transform flow types to flow Comment Types in output.E.g.
$ flow-remove-types --comment source.jsoutputs:
Closes #66.