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

Run rake tasks with interpreter warnings enabled #1177

Merged
merged 3 commits into from
Jun 17, 2019

Conversation

ashmaroli
Copy link
Contributor

Reaction to #1174 (comment)

We really should aim to get to 0 warnings because Rouge is extremely noisy.

/cc @mojavelinux

@pyrmont
Copy link
Contributor

pyrmont commented Jun 12, 2019

He wasn't kidding about it being noisy, huh?

@ashmaroli
Copy link
Contributor Author

He wasn't kidding about it being noisy, huh?

Hehe.. yeah. 😃
I think one easy way towards resolving this is by configuring RuboCop to convert regex to use the %r() syntax with varying delimiters..

@pyrmont
Copy link
Contributor

pyrmont commented Jun 13, 2019

I thought it simpler to convert /../ to %r/.../. Have submitted #1180 with Rubocop's ambiguity warnings enabled.

@pyrmont
Copy link
Contributor

pyrmont commented Jun 13, 2019

I've also played around with turning on Ruby's warnings as this PR does. #1180 helps reduce the number somewhat but there's still a lot. I'm having a look at how feasible it is to fix them by hand (maybe too time consuming and not worth the effort while so many substantive PRs are still outstanding).

@mojavelinux
Copy link
Contributor

I'd be happy to take a crack at fixing some of the warnings once you get to a stopping point.

@pyrmont
Copy link
Contributor

pyrmont commented Jun 13, 2019

@mojavelinux Thanks :) I'll let you know how I go.

@pyrmont pyrmont added needs-review The PR needs to be reviewed and removed discussion-open labels Jun 13, 2019
@ashmaroli
Copy link
Contributor Author

I volunteer to help with this if @mojavelinux needs a hand..

@ashmaroli
Copy link
Contributor Author

I thought it simpler to convert /../ to %r/.../.

I'm okay with whatever it takes to keep the boat afloat.. 😛

@mojavelinux
Copy link
Contributor

to keep the boat afloat...

...and be a good Ruby citizen.

@ashmaroli
Copy link
Contributor Author

Opened #1184 to remove all those warning: instance variable @all not initialized lines

Rakefile Outdated Show resolved Hide resolved
@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jun 17, 2019
@ashmaroli
Copy link
Contributor Author

@pyrmont I'm sticking to the disable warnings by default stance for now. Once we've reduced the warnings to a manageable level, I'll submit a PR to make it enabled by default..

@mojavelinux
Copy link
Contributor

👍 The important point is that they're simple to enable ;)

@ashmaroli
Copy link
Contributor Author

Thanks Dan 😃

@pyrmont pyrmont merged commit 9909742 into rouge-ruby:master Jun 17, 2019
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Jun 17, 2019
@ashmaroli ashmaroli deleted the warning-mode branch June 17, 2019 12:02
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.

3 participants