-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add support for raw mode #80
Add support for raw mode #80
Conversation
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.
Thank you so much for this.
This is probably the best PR I have gotten for this project in it's somewhat 20ish year streak in the open source :)
I did leave some comments here and there with some nits I'm also happy to do myself. The biggest one would be to use a variable to add new lines and change it after we parsed the options. Replace is super quick and wouldn't really add much performance issues but it's just cleaner coding. Happy to also do this myself but figured I chat to you first ;)
Hey, first of all, thanks for the kind words!! I'll write my input into your comments, but in general, all seem like good ideas, and I don't mind doing them. Regarding the newline variable: |
As per our discussion in #79 regarding the spaceless option in node: Changed the 'space' option to 'spaceless' in the nodejs implementation.
|
As we discussed in #79, the implementation was changed to use a variable that is set based on the relevant flags. |
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.
This looks great now!
Thank you
Looks like the action is failing due to old node versions? I have to experiment with this in another PR. Give me a day or two |
This PR is an attempt to add support for raw mode, as discussed here #79.
Quick overview of changes:
Hope everything looks good!