Add more file extensions #8

robertodr · 2017-08-10T07:24:15Z

I am not a Ruby expert, but I've managed to make the following changes:

The plugin now also checks files with extensions .hpp, .hh, .cxx, .cc, .cpp, hence addressing Support more file extensions (hpp, cpp, etc.) #2
The error message is more informative. It first lists the offending files, then suggests appropriate actions and eventually prints the patches to apply.
Generation of the more informative error message is now done differently.
Pass the list of extensions to check to the plugin, instead of hardcoding the valid extensions. Default behavior only checks Objective-C extensions (.h, .m, .mm)

You can see the plugin in action for this example repository: PCMSolver/danger-cpp-example#3

~~### Questions/Ideas~~

~~- [ ] I haven't added tests, because I don't know how to.~~
~~- [ ]Generating the more informative error message requires iterating twice over the changes array.~~

- The plugin now also checks files with extensions .hpp, .hh, .cxx, .cc, .cpp - The error message is more informative. It first lists the offending files, then suggests appropriate actions and eventually prints the patches to apply.

nikolaykasyanov · 2017-08-10T08:26:04Z

Looks great!

I believe it's a good start, a more generic approach (passing extensions as argument) can be implemented later.

Adding/editing tests is easy, most of them are here.

You might also want to run tests locally, to do that you need to install bundler gem and then do bundle install in the project directory. When it's done, run bundle exec rake spec to run tests.

When in doubt, please use to .travis.yml file as a reference.

Thank you for your contribution and feel free to ask anything.

robertodr · 2017-08-10T10:47:29Z

Thank for the pointers, @nikolaykasyanov. I have now fixed the existing tests and refactored the generation of the error message. I will try to add a tests for:

More than one failing file.
The new file extensions.

If you have suggestions on how to pass file extensions to be checked to the plugin, I can try to implement that too.

nikolaykasyanov · 2017-08-10T13:39:12Z

@robertodr regarding file extension list as argument:

we can do that by introducing a new key into plugin's configuration hash. At the time only ignore_file_patterns exists.

A new key could be introduced and its value could be used instead of hardcoded list of extensions.

Also, if this key is not specified in the config, current list of extensions must be used for backward compatibility reasons.

Does it make sense?

Plugin invocation could look like this:

code_style_validation.check ignore_file_patterns: [/^Pods\//] file_extensions: ['.h', '.m', ...]

robertodr · 2017-08-10T14:03:56Z

How does it look now? 😄 I modified one line in the tests, so that the new parameter is tested:

@my_plugin.check file_extensions: ['.h', '.m', '.mm', '.C', '.cpp']

Adding a new proper test for a .hpp and/or .cpp file is beyond my capabilities, however.

nikolaykasyanov · 2017-08-10T15:09:44Z

README.md

+This plugin uses 'clang-format' to look for code style violations in added
+lines on the current MR / PR, and offers inline patches.
+By default only Objective-C files, with extensions `.h`, `.m`, `.mm` and
+`.C`, are checked.


Why does .C have to be capitalized?

According to Wikipedia Objective-C files usually have those extensions. I only kept Objective-C source file extensions as defaults for the file_extensions input parameter.

Good find, I've never seen that extension in the wild, though. Also, it's effectively equal to .c extension on case-insensitive filesystems (i.e. on most Macs).

Yes, of course! I'll remove it and leave .m, .mm and .h as defaults.

nikolaykasyanov · 2017-08-10T15:22:47Z

@robertodr 👍 looks great.

It seems possible to make a test like existing "Reports code style violation as error", but such that does not specify .m as extension, thus making Danger to not emit an error. It could be called "Does not report errors in files with unwanted extensions" (not the best wording, though).

Another crucial test I'd expect is to verify that ignore_file_patterns are indeed respected when some of the ignored files have extensions matching file_extensions (for example, we ignore generated/ directory that contains .m file and we pass file_extensions: [ '.m' ], in this case generated/file.m should indeed be ignored). This can be achieved by adding explicit file_extensions argument to the invocation.

nikolaykasyanov · 2017-08-10T15:23:36Z

I could assist you with mentioned tests tomorrow if needed.

robertodr · 2017-08-10T15:25:32Z

That would be great. I might not have time to look at it tomorrow, though.

nikolaykasyanov · 2017-08-11T12:18:44Z

@robertodr could you please provide me a push access to this Pull Request so I can play with tests?

nikolaykasyanov · 2017-08-11T12:48:25Z

@robertodr nevermind, it seems that checkbox was already enabled.

nikolaykasyanov · 2017-08-11T12:53:07Z

lib/code_style_validation/plugin.rb

+      message = ''
+      unless offending_files.empty?
+        message = 'Code style violations detected in the following files:' + "\n"  
+	offending_files.each do |file_name|


You seem to be using tabs here. Please use spaces.

It could be helpful to configure Rubocop support in your editor. This repo doesn't have Rubocop's config yet, so it'll report some false positives, but it will also detect things like inconsistent indentation and trailing spaces.

nikolaykasyanov · 2017-08-11T12:54:00Z

lib/code_style_validation/plugin.rb

+
+      message = ''
+      unless offending_files.empty?
+        message = 'Code style violations detected in the following files:' + "\n"  


Trailing space.

Ooops... Sorry. I haven't yet reconfigured my Neovim properly after reinstalling the OS.

No worries!

nikolaykasyanov · 2017-08-11T14:43:47Z

My test fails on Travis, looking into it.

robertodr · 2017-08-14T17:00:34Z

My neovim configuration is back on track! I have fixed indentation inconsistencies and fixed #9
I also had a crazy idea, that I don't know how to implement. Usually, one PR might touch more than one or two files in many places. The patches might become quite long and the comment from the bot might easily take up a large portion of the screen and obfuscate comments from reviewers. Here comes the crazy idea, would it be possible to have the bot write a Gist with the patch? The comment from the bot would then:

List the offending files.
Suggest actions to correct the errors.
Point users to the Gist with the patch.

What do you think?

nikolaykasyanov · 2017-08-15T07:27:30Z

@robertodr thanks! I'll try this out in our internal environment for a couple of days before merging.

Don't you mind if I squash commits upon merging?

robertodr · 2017-08-15T08:53:58Z

Just test for as long as needed. No problem with squashing, as long as it preserves authorship assignment. What do you think about the crazy idea? Does it make sense? Is it doable?

nikolaykasyanov · 2017-08-15T09:51:34Z

@robertodr sorry, I didn't notice your big comment.

The idea about Gist sounds reasonable. The only concern is that it introduces a separate code path for open source projects (because posting diffs to Gist most certainly won't work for proprietary/closed source projects).

I wonder if GitHub (GitLab) supports some kind of folding, that might solve the problem of big patches cluttering the review.

Alternatively, patches could be attached as files (if GitHub API & Danger support that) and linked in comment text.

robertodr · 2017-08-15T11:18:41Z

Oh! Yes, of course. I had not thought about that. Well, this PR is complete. Anything else can be part of subsequent PRs. Would you agree to tag the code and mint a release when the PR is merged?

nikolaykasyanov · 2017-08-18T09:18:48Z

@robertodr here you go: https://github.com/flix-tech/danger-code_style_validation/releases/tag/0.1.0

Thanks for your contribution!

robertodr · 2017-08-18T09:19:26Z

Thanks! It's been a pleasure collaborating with you on this.

Add more file file extensions

30f4c41

- The plugin now also checks files with extensions .hpp, .hh, .cxx, .cc, .cpp - The error message is more informative. It first lists the offending files, then suggests appropriate actions and eventually prints the patches to apply.

nikolaykasyanov self-requested a review August 10, 2017 08:22

nikolaykasyanov self-assigned this Aug 10, 2017

Fix tests, refactor message generation

5ca556e

Generate offending_files and patches arrays properly

6930556

Allow user to specify file extensions to check

af2b349

nikolaykasyanov suggested changes Aug 10, 2017

View reviewed changes

Adds some tests for file extension configuration

e221856

nikolaykasyanov reviewed Aug 11, 2017

View reviewed changes

Adds code style violation message verification

3ae72a4

Nikolay Kasyanov added 2 commits August 11, 2017 16:58

Adds .clang-format to make diffs produced in tests deterministic

b9537b1

Simplifies .clang-format & updates expected Markdown text

1571793

robertodr changed the title ~~Add more file file extensions~~ Add more file extensions Aug 12, 2017

Address @nikolaykasyanov review, fix #9

eb1ca60

Install clang-format-3.8 also on Linux

dfaae35

nikolaykasyanov approved these changes Aug 15, 2017

View reviewed changes

Joins patches with linebreak to make headers render properly

7eb737a

nikolaykasyanov merged commit 5c250d9 into flix-tech:master Aug 18, 2017

nikolaykasyanov mentioned this pull request Aug 18, 2017

Support more file extensions (hpp, cpp, etc.) #2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more file extensions #8

Add more file extensions #8

robertodr commented Aug 10, 2017 •

edited

Loading

nikolaykasyanov commented Aug 10, 2017

robertodr commented Aug 10, 2017 •

edited

Loading

nikolaykasyanov commented Aug 10, 2017 •

edited

Loading

robertodr commented Aug 10, 2017

nikolaykasyanov Aug 10, 2017

robertodr Aug 10, 2017 •

edited

Loading

nikolaykasyanov Aug 10, 2017

robertodr Aug 10, 2017

nikolaykasyanov commented Aug 10, 2017

nikolaykasyanov commented Aug 10, 2017

robertodr commented Aug 10, 2017

nikolaykasyanov commented Aug 11, 2017

nikolaykasyanov commented Aug 11, 2017

nikolaykasyanov Aug 11, 2017

nikolaykasyanov Aug 11, 2017

nikolaykasyanov Aug 11, 2017

robertodr Aug 11, 2017

nikolaykasyanov Aug 11, 2017

nikolaykasyanov commented Aug 11, 2017

robertodr commented Aug 14, 2017 •

edited

Loading

nikolaykasyanov commented Aug 15, 2017

robertodr commented Aug 15, 2017

nikolaykasyanov commented Aug 15, 2017 •

edited

Loading

robertodr commented Aug 15, 2017

nikolaykasyanov commented Aug 18, 2017

robertodr commented Aug 18, 2017

Add more file extensions #8

Add more file extensions #8

Conversation

robertodr commented Aug 10, 2017 • edited Loading

nikolaykasyanov commented Aug 10, 2017

robertodr commented Aug 10, 2017 • edited Loading

nikolaykasyanov commented Aug 10, 2017 • edited Loading

robertodr commented Aug 10, 2017

Choose a reason for hiding this comment

robertodr Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolaykasyanov commented Aug 10, 2017

nikolaykasyanov commented Aug 10, 2017

robertodr commented Aug 10, 2017

nikolaykasyanov commented Aug 11, 2017

nikolaykasyanov commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolaykasyanov commented Aug 11, 2017

robertodr commented Aug 14, 2017 • edited Loading

nikolaykasyanov commented Aug 15, 2017

robertodr commented Aug 15, 2017

nikolaykasyanov commented Aug 15, 2017 • edited Loading

robertodr commented Aug 15, 2017

nikolaykasyanov commented Aug 18, 2017

robertodr commented Aug 18, 2017

robertodr commented Aug 10, 2017 •

edited

Loading

robertodr commented Aug 10, 2017 •

edited

Loading

nikolaykasyanov commented Aug 10, 2017 •

edited

Loading

robertodr Aug 10, 2017 •

edited

Loading

robertodr commented Aug 14, 2017 •

edited

Loading

nikolaykasyanov commented Aug 15, 2017 •

edited

Loading