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 option to check inline comments? #62

Closed
Pharb opened this issue Mar 1, 2017 · 4 comments
Closed

Add option to check inline comments? #62

Pharb opened this issue Mar 1, 2017 · 4 comments

Comments

@Pharb
Copy link

Pharb commented Mar 1, 2017

In issue #49 the reason inline comments are not supported seems to be the risk of false-positives e.g. a string that contains text which looks like a comment.

Would you maybe accept a pull request that adds an option to include possible inline TODOs?
There could be a warning in the documentation about the risk of false-positives.

Alternatively could there be an option to allow an external parser, e.g. passing a function to associateExtWithParser?

var inlineParser = require('my-inline-parser');

[...].associateExtWithParser({
    '.py': {
        parser: inlineParser
    }
})

Which of these two choices would you prefer? Or do you have another possible solution?

@pgilad
Copy link
Owner

pgilad commented Mar 1, 2017

I see this feature keeps on getting requested, so it must be needed ;-)
My idea is to make this opt-in with options. Off course we will try to reduce false positives, but the user understands it impossible to hermitically proof it (and also expensive).

So each parser receives params:
https://github.com/pgilad/leasot/blob/master/lib/parsers/defaultParser.js#L9

And inside we can start adding support for inline comments using an opt-in option for the parse that we can call whatever we want - parseInlineComments: true for example.

If we see that we can minimize the false positives to almost zero - we can remove the option and always parse for inline comments.

What do you think?

@Pharb
Copy link
Author

Pharb commented Mar 1, 2017

That sounds good and would be sufficient for my use cases.

Do the params apply to all parsers? Would then every supported parser have to implement the inline option? Or would it make sense to selectively enable this only for certain parsers?

One advantage of my other suggestion about allowing external parser plugins would be the possibility to independently create and use experimental parsers, e.g. an AST based JavaScript parser. But maybe this would then be another issue.

@pgilad
Copy link
Owner

pgilad commented Mar 1, 2017

Yes I really like the idea of using an external parsers! It would require a change in this lines:
https://github.com/pgilad/leasot/blob/master/lib/parsers.js#L111-L112

But then allow for ultimate flexibility with parsers, and people can add any parser they like to replace...

@pgilad
Copy link
Owner

pgilad commented Mar 1, 2018

So this issue is quite old, I'm closing it, feel free to re-open if you feel it's still relevant.

I'm for dedicated parsers (external to this project - but being able to plug them in for results), but not for overly-engineered regex which are very fragile

@pgilad pgilad closed this as completed Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants