-
Notifications
You must be signed in to change notification settings - Fork 52
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if both --pretty
and --comment
are specified?
@@ -29,6 +29,16 @@ echo "Test: flow-remove-types --pretty --sourcemaps inline test/source.js" | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
,--comment
options to Transform flow types to flow Comment Types in output.E.g.
$ flow-remove-types --comment source.js
outputs:
Closes #66.