Skip to content
This repository was archived by the owner on Jul 19, 2025. It is now read-only.

Conversation

@pointlessone
Copy link
Contributor

Example config:

engines:
  duplication:
    enabled: true
    config:
      languages:
        foolang:
          patterns:
          - "*.foofile"
          - "Foofile"

Copy link
Contributor

@wfleming wfleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good! Nice that the diff is so small.

Aside from my comment about a couple unit tests being nice to have, a larger question: the README here covers most config options, so adding README content for this new option is probably appropriate. Maybe a question for @gordondiggs.

Related to the docs question: in your examples it looks like you've been using simple single-star-with-extension patterns. If we do write docs here it's probably important to make clear that most patterns should be prefixed by **/: all of the current hard-coded patterns do, and without that I think a pattern will only match files in the root dir of the repo.

end

def patterns_for(language, fallbacks)
Array(fetch_language(language).fetch("patterns", fallbacks))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice simple way of ensuring you get the type you want, cool. 👍

end
end

def patterns_for(language, fallbacks)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The integration-y test you have is great. Would it be reasonable to get some unit tests as well? For example, in addition to the happy path, it'd be nice to have a test that a malformed config like:

ruby:
  patterns: "*.rb"

does the right thing. (I think your code does handle this quite nicely, but guarding against future regressions with a test would be nice.)


def parse
runner = CommandLineRunner.new("php #{parser_path}")
runner = CommandLineRunner.new("php -d 'display_errors = Off' #{parser_path}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good bugfix in its own right, and not directly related to the new feature. It's already in its own commit, so I'd support cherry-picking this into a new branch & shipping it separately as its own PR if you want to.

Also, it's been many years since I wrote much PHP, so pardon if this is a silly question: this won't stop us from seeing exceptions/stack traces in logs if the parser crashes, will it?

Copy link
Contributor Author

@pointlessone pointlessone Jan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP duplication specs were failing for me because notices polluted the output. This is definitely setup issue but I don't know if CI has similar setup to that of mine. I can extract this commit and see how CI will react to that.

It might be that without this change CI will fail this PR build.

@gdiggs gdiggs merged commit e2535bc into codeclimate:master Jan 27, 2017
@pointlessone pointlessone deleted the patterns branch January 27, 2017 20:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants