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

feat: added newline formatting for comments #251

Merged
merged 12 commits into from Jun 17, 2020

Conversation

anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Jun 6, 2020

fixes #250

Before

<a>as</a>
<!--   multi 
 line      -->

<!-- mm -->

<!-- 
  on
-->

After

<a>as</a>
<!-- 
    multi 
 line       
 -->

<!-- mm -->

<!-- on -->

@anikethsaha anikethsaha requested a review from Scrum June 6, 2020 18:02
Copy link
Member

@Scrum Scrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about making this functionality an option?

src/index.js Outdated
const commentsFormatting = tree => {
tree.walk = walk;
tree.walk(node => {
if (typeof node === 'string' && /<!--[^]*-->/gm.test(node.trim())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Regular expression /<!--[^]*-->/gm is better to put in a constant
  2. The beginning <!-- and the end --> of the notation for the comment I would put into a constant

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea cool 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. The beginning <!-- and the end --> of the notation for the comment I would put into a constant

DOne 👍

  1. Regular expression /<!--[^]*-->/gm is better to put in a constant

I dont know why but when I am putting that into a constant and using the .test method, its not working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you create with new RegExp ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, its still the same. I don't know why it's happening. Its working fine for the other regex constants

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm yes that's weird

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated
tree.walk(node => {
if (typeof node === 'string' && /<!--[^]*-->/gm.test(node.trim())) {
const originalComments = node.trim();
const content = originalComments.replace(/^<!--/g, '').replace(/-->$/, '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, it seems to me that a regular expression that takes into account a multiline is better - <!--([\s\S]*?)--> content will match

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pay attention to this post here is a coincidence and there will be our content, and we do not need to replace

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didnt get this 🔼

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@anikethsaha
Copy link
Member Author

What do you think about making this functionality an option?

yea we can, can you suggest the option name ? may be formatComment ?

@Scrum
Copy link
Member

Scrum commented Jun 8, 2020

yea we can, can you suggest the option name ? may be formatComment ?

I like it better when there is narrowness first and then its options, for example

commentTrim, commentFormat etc a good name on the run until you come up with))

@anikethsaha
Copy link
Member Author

commentFormat sounds good

@anikethsaha anikethsaha requested a review from Scrum June 14, 2020 11:16
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@anikethsaha
Copy link
Member Author

about

node = node?.trim()

node won't be undefined, just that we need the type of the node to be string in order to do the trim,

I guess optional chaining doesnt work the type , it just checks the whether its undefined or not, right !.

Let me know if I missed anything.

@Scrum
Copy link
Member

Scrum commented Jun 15, 2020

I guess optional chaining doesnt work the type , it just checks the whether its undefined or not, right !.

Yes, in this case only .trim()

@anikethsaha
Copy link
Member Author

I guess optional chaining doesnt work the type , it just checks the whether its undefined or not, right !.

Yes, in this case only .trim()

but with node?.trim() it will try to trim the object type as well which will be an error as object doesnt have trim method.

@Scrum
Copy link
Member

Scrum commented Jun 15, 2020

I guess optional chaining doesnt work the type , it just checks the whether its undefined or not, right !.

Yes, in this case only .trim()

but with node?.trim() it will try to trim the object type as well which will be an error as object doesnt have trim method.

Ohhh yes, I think I missed this little detail of the fact that the first condition is string checking

I think then it will be better if we put out the regular expression test in the condition

if (typeof(node) === 'string') {
  node = node.trim()
  if (...) {...}
}

@anikethsaha anikethsaha requested a review from Scrum June 16, 2020 18:59
@Scrum
Copy link
Member

Scrum commented Jun 17, 2020

@anikethsaha Hi, reviewers please ))

@anikethsaha
Copy link
Member Author

Looks good to me. 🎉

@Scrum
Copy link
Member

Scrum commented Jun 17, 2020

@anikethsaha Good job, thanks

@Scrum Scrum merged commit 659daa1 into posthtml:master Jun 17, 2020
@anikethsaha anikethsaha deleted the feat-comment-formatting branch June 17, 2020 08:20
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.

[Feature Request]: comments formatting
2 participants