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

Add support for raw mode #80

Merged

Conversation

FriedlandAaron
Copy link
Contributor

This PR is an attempt to add support for raw mode, as discussed here #79.

Quick overview of changes:

  • added --raw-mode and -r flags to both node and rust APIs
  • implemented raw mode in both node and rust APIs
  • updated relevant tests to reflect the flag addition
  • updated relevant tests to reflect the implementation
  • added new tests for the implementation

Hope everything looks good!

Copy link
Owner

@dominikwilkowski dominikwilkowski left a 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 ;)

nodejs/src/Render.js Outdated Show resolved Hide resolved
rust/src/render.rs Outdated Show resolved Hide resolved
rust/src/render.rs Outdated Show resolved Hide resolved
rust/src/render.rs Show resolved Hide resolved
rust/src/config.rs Outdated Show resolved Hide resolved
rust/src/args.rs Outdated Show resolved Hide resolved
nodejs/test/unit/Render.spec.js Outdated Show resolved Hide resolved
@FriedlandAaron
Copy link
Contributor Author

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:
I was considering just adding a function that received the rawmode flag and returned the appropriate newline char, but I ended up going with the current solution because of the inconsistency I mentioned here #79. A variable would also work, like you suggest, but it would still mean slightly different changes in different locations. Not a huge deal, but I decided not to do it, in favor of changing as little of the implementation as possible. I'll go into detail in the issue conversation #79.

@FriedlandAaron
Copy link
Contributor Author

As per our discussion in #79 regarding the spaceless option in node:

Changed the 'space' option to 'spaceless' in the nodejs implementation.

  • updated the Options object
  • updated the tests

@FriedlandAaron
Copy link
Contributor Author

As we discussed in #79, the implementation was changed to use a variable that is set based on the relevant flags.
Tests pass when I run them locally.

Copy link
Owner

@dominikwilkowski dominikwilkowski left a 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

rust/src/render.rs Show resolved Hide resolved
@dominikwilkowski
Copy link
Owner

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

@dominikwilkowski dominikwilkowski merged commit e00f0fa into dominikwilkowski:released Jun 13, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants