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

Fix check line alignment #638

Closed
wants to merge 3 commits into from

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Sep 16, 2020

Some fixes for the rule check-line-alignment.

It skips single line comments and considers types with spaces.

@brettz9
Copy link
Collaborator

brettz9 commented Sep 16, 2020

I can take a look at this, but I'm wondering if you might be open to adding support for an option to preserve whitespace in comment-parser--which is a small library that we are elsewhere using in our codebase and has been open to PRs?

I'd feel a lot more comfortable if the rule were to instead total up whitespace counts based on parsed results rather than making ever longer, if not also harder-to-maintain or imperfect, regular expressions.

This could also be potentially useful for our other rules, including fixers where we use its parser and stringifier to make modifications and where, without such whitespace-awareness, the fixers may thus drop whitespace that the user might have wanted to preserve.

Thanks!

@renatho
Copy link
Contributor Author

renatho commented Sep 16, 2020

I can take a look at this, but I'm wondering if you might be open to adding support for an option to preserve whitespace in comment-parser--which is a small library that we are elsewhere using in our codebase and has been open to PRs?

I'd feel a lot more comfortable if the rule were to instead total up whitespace counts based on parsed results rather than making ever longer, if not also harder-to-maintain or imperfect, regular expressions.

This could also be potentially useful for our other rules, including fixers where we use its parser and stringifier to make modifications and where, without such whitespace-awareness, the fixers may thus drop whitespace that the user might have wanted to preserve.

Thanks!

Interesting! Yep! I'll try to take a look at this on the next days. :)

@renatho
Copy link
Contributor Author

renatho commented Nov 26, 2020

@brettz9, I wonder if we could merge this fix for now, and when the comment-parser changes are done, we update this rule to use it.

Considering that probably it can take a while, I think it'd be nice to already fix this issue here. WDYT?

@brettz9
Copy link
Collaborator

brettz9 commented Nov 28, 2020

@renatho : I really don't want to introduce new code that has to be maintained with its own complexities. It is not going super fast, I know, but the project maintainer has been making progress on this new release and has been receptive to changes in the past, so I'd hold out a little bit longer. We will need those trim and indent features so if they don't get added in his new release, you could help expedite things by adding a PR to add them back here (or if it turns out too inefficient or cumbersome to do them post-parsing/post-stringifying, then maybe to propose a generic hook for comment-parser so we can supply a callback to do the trimming, etc.).

@renatho
Copy link
Contributor Author

renatho commented Nov 29, 2020

@renatho : I really don't want to introduce new code that has to be maintained with its own complexities. It is not going super fast, I know, but the project maintainer has been making progress on this new release and has been receptive to changes in the past, so I'd hold out a little bit longer. We will need those trim and indent features so if they don't get added in his new release, you could help expedite things by adding a PR to add them back here (or if it turns out too inefficient or cumbersome to do them post-parsing/post-stringifying, then maybe to propose a generic hook for comment-parser so we can supply a callback to do the trimming, etc.).

Hey @brettz9! I get your point. But I was thinking that maybe it could be nice to merge this one only because it's more a fix than a new feature. But if you prefer to wait a little more for this, it's okay. ;)

@gajus
Copy link
Owner

gajus commented Jan 11, 2021

🎉 This issue has been resolved in version 31.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Jan 11, 2021
@brettz9
Copy link
Collaborator

brettz9 commented Jan 11, 2021

I've gone ahead and applied your changes. For future changes, please note that comment-parser has now been updated in a way where we should be able to modify its parsed tokens and stringify them to achieve alignment changes.

Note that comment-parser was only recently updated, so it may yet add tweaks and improvements which make it easier to stringify. Currently if one makes changes on the tokens property of the objects in its parsed Block.tags[].source array, one has to copy these changes to the Block.source array (which mirrors the tag source along with holding non-tag source such as the jsdoc block description (so that full jsdoc block stringifying can be done).

@renatho
Copy link
Contributor Author

renatho commented Jan 11, 2021

I've gone ahead and applied your changes. For future changes, please note that comment-parser has now been updated in a way where we should be able to modify its parsed tokens and stringify them to achieve alignment changes.

Note that comment-parser was only recently updated, so it may yet add tweaks and improvements which make it easier to stringify. Currently if one makes changes on the tokens property of the objects in its parsed Block.tags[].source array, one has to copy these changes to the Block.source array (which mirrors the tag source along with holding non-tag source such as the jsdoc block description (so that full jsdoc block stringifying can be done).

Hey @brettz9! Thank you for the quick update of the comment-parser dependency! I want to update the rule to use the comment-parser as soon as I can!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants