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

That stolen figurine is omnipresent, like candy #2573

Merged
merged 19 commits into from
Jan 5, 2016

Conversation

alexdowad
Copy link
Contributor

@jonas054, could you please look at 8c36731? (I think you are the author of the caching code.)

There was a spec there checking that text which is invalid under UTF-8 encoding could still be serialized out and in. It used a comment with some invalid text... but since comments are not cached any more, I moved the invalid text to the offense description instead. Unfortunately... the test doesn't pass.

Any idea on the best way to fix this?

…ing multiple lines

For example:

    if condition1 &&
       condition2 &&
       condition3
      return
    end

This will look funny if corrected to:

    return if condition1 &&
              condition2 &&
              condition3

Therefore, leave these cases to the discretion of individual programmers rather
than requiring the 'guard clause' style with modifier if/unless to be used.
@alexdowad alexdowad force-pushed the that_stolen_figurine branch 3 times, most recently from 1bee23d to 95e459d Compare January 4, 2016 13:23
The cached disabled line ranges and comments are used to add UnneededDisable
offenses to the cached offenses. But this process is deterministic, so we can
simply do it *before* caching and then just cache the resulting offenses.

This greatly simplifies the caching code. As a bonus, the caching should be
more effective, since more work is done once only and then reused, rather
than being repeated every time the cached data is retrieved.
Under certain circumstances, Lint/UnneededDisable's autocorrection would bite
off just a bit too much, and delete a space which was needed. My bad.
When only some cops in a rubocop:disable comment were unneeded, and the comment
was not at the very beginning of the file, random junk would appear as the
highlight, rather than the unneeded content.
…LambdaCall's toes

A `#call` method can be called using this special syntax:

    obj.()

`Style/LambdaCall` checks for this syntax, and depending on its enforced style,
either enforces or forbids its use.

Since `Style/LambdaCall` is already handling these cases,
`Style/MethodCallParentheses` can just stay away.
@alexdowad
Copy link
Contributor Author

I have fixed the problem with serializing invalid UTF-8 text in CachedData. (Just scrub the string of invalid bytes before serializing it out.)

The commit which adds --color is just an updated version of the one which I sent a PR for previously. The objection which @jonas054 raised the first time around is that it changes the signature of the constructor for Formatter::Base, which will break custom formatters.

This is a very real issue; and yet, after further consideration, I still feel this is a better solution than poking config options in using setters like #color=. We will likely want to pass more optional data into formatters in the future; this provides a way to do so, without having to break backwards compatibility again.

However, if the maintainers disagree, I am happy to re-implement in another way.

@alexdowad
Copy link
Contributor Author

Added a couple of missing CHANGELOG entries. Ready for final review.

@alexdowad alexdowad force-pushed the that_stolen_figurine branch 2 times, most recently from f6f35a5 to d370e19 Compare January 4, 2016 20:46
This cop finds things like:

    if condition_a
      action_a
    else
      if condition_b
        action_b
      end
    end

Which should be converted to:

    if condition_a
      action_a
    elsif condition_b
      action_b
    end
… modifier if

...or unless. The problem occurred when a redundant merge had more than 1
key-value pair, and a modifier `if`, like:

    hash.merge!(a: 1, b: 2) if condition

That was erroneously auto-corrected to:

    hash[:a = 1]
    hash[:b = 2] if condition
We had no tests checking whether `Lint/EndAlignment`'s `variable` style worked
or not when a node with `end` is not on the RHS of an assignment. So it was
never noticed when 31d0b31 completely broke the `variable` style.

Fix it up, and add tests so this doesn't happen again.
Previously, when the `variable` style was used, auto-correction gave the
wrong results on the RHS of an ivar assignment, cvar assignment, and so on.
@alexdowad
Copy link
Contributor Author

I have a feeling this check exists in other places in the code. Might be a good idea to move up this helper method.

Hmm. I've searched and haven't found any equivalent method which I could unify this one with.

We do have a lot of duplicate helper methods in RC, though, and I do want to de-duplicate more of them as we go.

CachedData uses the JSON library. It is already required in json_formatter.rb,
but it is better to require it here as well, since it is needed here.
As suggested by Jordon Bedwell.
@alexdowad
Copy link
Contributor Author

Updated according to @bbatsov's review. There are no problematic "magic numbers" for target Ruby version any more (see 12c6efc).

Added a commit which adjusts the default TargetRubyVersion to 2.0.

I was thinking of adding a cache busting value somewhere in ResultCache. I see that @jonas054 is already using the source for bin/rubocop for cache busting... but this source hasn't changed, and the format of the cached data has.

I think RuboCop::Version::STRING should be used for cache busting. Then it might not be necessary to read the source for bin/rubocop. What do you think, @jonas054?

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 5, 2016

I think RuboCop::Version::STRING should be used for cache busting. Then it might not be necessary to read the source for bin/rubocop. What do you think, @jonas054?

This will only work for people using the stable release and I know there are users tracking the master branch (e.g. users impatient to get bug fixes, support for Ruby 2.3). For this to work we should start issuing some pre-releases.

bbatsov added a commit that referenced this pull request Jan 5, 2016
That stolen figurine is omnipresent, like candy
@bbatsov bbatsov merged commit 7bd0f11 into rubocop:master Jan 5, 2016
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 5, 2016

Anyways, this PR is good shape, so I'll merge it. We can figure out the cache-busting issue later.

@alexdowad
Copy link
Contributor Author

Another solution to cache busting would be to put a VERSION constant in ResultCache, which would be incremented every time we make a change which invalidates old cache files.

@alexdowad alexdowad deleted the that_stolen_figurine branch January 5, 2016 07:33
@jonas054
Copy link
Collaborator

jonas054 commented Jan 6, 2016

@alexdowad The reason why the cache invalidation logic uses a checksum for the entire RuboCop source (not just bin but also lib) is that it has to function when we're working on RuboCop. For other users a version constant would probably do, but I don't see how it could work for us RuboCop developers.

@alexdowad
Copy link
Contributor Author

@jonas054 I didn't get that it was reading the source in lib as well. Thanks. In that case we won't have a problem with the changed format.

If we wanted to avoid the overhead of calculating the source checksum, we could just disable caching in RC's own .rubocop.yml, then use the version constant. I'm just saying; I don't actually intend to do anything about this.

@jonas054
Copy link
Collaborator

jonas054 commented Jan 6, 2016

Yes. There are some cases where we can actually take advantage of caching during RuboCop development, and that's when you're only changing specs. So let's keep it for now.

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.

4 participants