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

Categorize 'interpreted as argument prefix' syntax lints #581

Closed
wants to merge 1 commit into from

Conversation

nevir
Copy link
Contributor

@nevir nevir commented Oct 20, 2013

I like to be able to send splats & blocks to DSL-style methods, but the parser dislikes that

@yujinakayama
Copy link
Collaborator

In configuration, EnforceArgumentPrefixParentheses under Syntax is a bit strange. It's just internal implementation matter with Parser. I think such a structure does not need to be exposed to users.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 21, 2013

@yujinakayama Yeah, I don't like the current implementation either. Not sure how we can improve the situation, though. Maybe we could have some generic config option to suppress certain warnings? I guess we should think about that for a while.

@nevir
Copy link
Contributor Author

nevir commented Oct 21, 2013

It might be clearer to just let the user specify regexes to filter it in
the config
On Mon, Oct 21, 2013 at 0:26, Bozhidar Batsov notifications@github.com
wrote:

@yujinakayama https://github.com/yujinakayama Yeah, I don't like the
current implementation either. Not sure how we can improve the situation,
though. Maybe we could have some generic config option to suppress certain
warnings? I guess we should think about that for a while.


Reply to this email directly or view it on GitHubhttps://github.com//pull/581#issuecomment-26697120
.

@nevir
Copy link
Contributor Author

nevir commented Oct 21, 2013

Or, separate cop per category of parser warning?
On Mon, Oct 21, 2013 at 7:23, Ian MacLeod ian.nevir.net@gmail.com wrote:

It might be clearer to just let the user specify regexes to filter it in
the config
On Mon, Oct 21, 2013 at 0:26, Bozhidar Batsov notifications@github.com
wrote:

@yujinakayama https://github.com/yujinakayama Yeah, I don't like the
current implementation either. Not sure how we can improve the situation,
though. Maybe we could have some generic config option to suppress certain
warnings? I guess we should think about that for a while.


Reply to this email directly or view it on GitHubhttps://github.com//pull/581#issuecomment-26697120
.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 21, 2013

Separate cop for parser warning seems reasonable. I think there are only a few parser warnings, anyways. I'll leave it up to @yujinakayama to decide how to proceed, since he's the keeper of the Syntax cop :-)

@yujinakayama
Copy link
Collaborator

Sorry for the late reply.

I also think separating cop for each category is simple and reasonable.

@nevir If you'd like to do this for interpreted as argument prefix, please reimplement and update this PR. Then I'll do other categories.

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 8, 2013

@nevir Ping :-)

@nevir
Copy link
Contributor Author

nevir commented Nov 8, 2013

Sorry, haven't had a chance to work on it yet - week or so!
On Fri, Nov 8, 2013 at 11:46, Bozhidar Batsov notifications@github.com
wrote:

@nevir https://github.com/nevir Ping :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/581#issuecomment-28077521
.

@nevir
Copy link
Contributor Author

nevir commented Nov 25, 2013

I haven't forgotten about this.

I've got a pull request out to parser to directly expose their reasons as symbols, which should greatly simplify the code here.

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 26, 2013

@nevir Good work. I guess we now have to wait for Parser 2.1, before we can wrap this.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 3, 2013

@nevir I guess you can bump the parser dep to ~> 2.1.0pre2 and do the necessary changes in RuboCop now.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 27, 2013

@yujinakayama I guess you'll have to do the outstanding changes.

@yujinakayama
Copy link
Collaborator

Sure.

@yujinakayama yujinakayama mentioned this pull request Jan 6, 2014
@bbatsov bbatsov closed this in #719 Jan 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants