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

coala command: use --limit-files #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fleimgruber
Copy link
Contributor

Uses --limit-files instead of --files. Also, additional line-start
and line-end regexp patterns.

@sils
Copy link
Member

sils commented Jul 22, 2017

Comment on 0fd5944, file test/resources/language/pep8.py, line 1.

The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section python.

The issue can be fixed by applying the following patch:

--- a/test/resources/language/pep8.py
+++ b/test/resources/language/pep8.py
@@ -1 +1 @@
-a = 1 + 1 # <- not PEP8 compliant
+a = 1 + 1  # <- not PEP8 compliant

@fleimgruber
Copy link
Contributor Author

@sils So this is the separate PR mentioned in #17. This is really how coala should be called in this case - I even vaguely remember you telling me just that during the sprint.

The issue now is that the PEP8 test file causes coala to complain when checking the repo itself. Can you somehow force this to be accepted despite that?

@@ -6,3 +6,4 @@ bears = GitCommitBear

[python]
bears = PEP8Bear
files = **.py
Copy link
Member

Choose a reason for hiding this comment

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

as per gitmate result #18 (comment) maybe put the test files into an ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see 875adbc which ignores the test file from root and includes it in the test subfolder (so that cask exec ert-runner picks it up). Test works, a local coala run from the root ignores the test file, but gitmate is still complaining. What am I missing? Is there a separate gitmate config?

@gitmate-bot
Copy link

Comment on 0fd5944, file test/resources/language/pep8.py, line 1.

The code does not comply to PEP8.

Origin: PEP8Bear, Section: python.

The issue can be fixed by applying the following patch:

--- a/test/resources/language/pep8.py
+++ b/test/resources/language/pep8.py
@@ -1 +1 @@
-a = 1 + 1 # <- not PEP8 compliant
+a = 1 + 1  # <- not PEP8 compliant

@9r0x
Copy link

9r0x commented Nov 6, 2017

Anyone working on this repo now? I guess 'Default' is deprecated now.

@jayvdb jayvdb changed the title Coala command: use --limit-files coala command: use --limit-files Nov 7, 2017
@jayvdb
Copy link
Member

jayvdb commented Nov 7, 2017

ping @fleimgruber

@fleimgruber
Copy link
Contributor Author

Not currently working on it. Will have a look in the next days.

@fleimgruber
Copy link
Contributor Author

@Grox-Ni the last commit already contains that deprecation.

@fleimgruber
Copy link
Contributor Author

FYI, I am using this on a daily basis and just tried with the most recent PyPI dev version and that works as well. AFAICS, the remaining problem is that gitmate-bot does not honour the "ignore" directive, see also #18 (comment).

@9r0x
Copy link

9r0x commented Nov 9, 2017

@fleimgruber you are right, I have an idea to solve gitmate's problem, please see #20, do you think I should reopen it?

@fleimgruber
Copy link
Contributor Author

@Grox-Ni No. What @jayvdb said in #20 (comment) and following.

@nemani
Copy link
Member

nemani commented Feb 24, 2018

@fleimgruber Are you still on this? Or should I close this? :)

@fleimgruber
Copy link
Contributor Author

@nemaniarjun this is still waiting for feedback on CI failures because of gitmate-bot, see #18 (comment).

@nemani
Copy link
Member

nemani commented Feb 24, 2018

@fleimgruber @Makman2 I am confused here, which config does gitmate bot use, the one in the PR or the one on master? Also can't we just override the CI so that we can get this merged? (I mean if gitmate uses the coafile from master then we should just merge this)

@Makman2
Copy link
Member

Makman2 commented Mar 17, 2018

I believe it takes it from the PR, otherwise you can't fix issues once they are on master.

@Makman2
Copy link
Member

Makman2 commented Mar 17, 2018

However I don't see a problem adding the test files to the ignore setting into the coafile if I see it correctly, should be quite straightforward 👍

@Makman2
Copy link
Member

Makman2 commented Mar 17, 2018

Ah sorry haven't read it properly. Let's try to rebase and see if it works. If not, we have to investigate.

@Makman2
Copy link
Member

Makman2 commented Mar 17, 2018

@gitmate-bot rebase

@Makman2
Copy link
Member

Makman2 commented Mar 17, 2018

Once more, gitmate was disabled.

@Makman2
Copy link
Member

Makman2 commented Mar 17, 2018

@gitmate-bot rebase

@gitmate-bot
Copy link

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@gitmate-bot
Copy link

Automated rebase with GitMate.io was successful! 🎉

@Makman2
Copy link
Member

Makman2 commented Mar 17, 2018

Hm seems the commits are already up-to-date, could you regenerate the commit hash @fleimgruber and repush? --> git commit --amend --no-edit && git push -f

@gitmate-bot
Copy link

Comment on 0fd5944, file test/resources/language/pep8.py, line 1.

The code does not comply to PEP8.

Origin: PEP8Bear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpan329k2u/test/resources/language/pep8.py
+++ b/tmp/tmpan329k2u/test/resources/language/pep8.py
@@ -1 +1 @@
-a = 1 + 1 # <- not PEP8 compliant
+a = 1 + 1  # <- not PEP8 compliant

@fleimgruber
Copy link
Contributor Author

Done. gitmate still complains.

@Makman2
Copy link
Member

Makman2 commented Apr 25, 2018

Sorry for responding that late, I've become a bit busy and haven't worked through my notification queue^^

And I know the cause: I've actually missed your very first commit in this PR, because it was at the very top of this page. You have to squash the commits, gitmate will do its analysis on each commit and uses the repository state of it. So your commit that added the new filenames in .coafile does not ignore the test file yet and flags a problem.

@fleimgruber
Copy link
Contributor Author

fleimgruber commented Apr 25, 2018

@Makman2 well, how obvious it is in hindsight... done and gitmate does not complain anymore.

:error-patterns ((error (or "None" line) ":" (or "None" column) ":MAJOR:" (message))
(warning (or "None" line) ":" (or "None" column) ":NORMAL:" (message))
(info (or "None" line) ":" (or "None" column) ":INFO:" (message)))
:command ("coala" "--format" "{line}:{column}:{severity_str}:{message}" "--find-config" "--limit-files" source-original)
Copy link
Member

Choose a reason for hiding this comment

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

But won't limit-files filter out given files? Wouldn't this render the plugin completely useless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

04adb14 changed --limit-files to --files. The --limit-files was put in 88315e6. Also, appeal to authority: During the hackathon, this was suggested to me by @sils.

:command ("coala" "--format" "{line}:{column}:{severity_str}:{message}" "--find-config" "--limit-files" source-original)
:error-patterns ((error line-start (or "None" line) ":" (or "None" column) ":MAJOR:" (message) line-end)
(warning line-start (or "None" line) ":" (or "None" column) ":NORMAL:" (message) line-end)
(info line-start (or "None" line) ":" (or "None" column) ":INFO:" (message) line-end))
Copy link
Member

Choose a reason for hiding this comment

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

Don't you have to adapt the coala --format to also emit line-starts and line-ends?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See :error-patterns documentation and example in http://www.flycheck.org/en/latest/developer/developing.html#writing-the-checker. The line-start and line-end are rx pattern elements and do not map to checker command results.

@Makman2
Copy link
Member

Makman2 commented Apr 28, 2018

  1. Commit message:

    • Ccoala :D
    • uUse
  2. I see two commits here:

    Uses --limit-files instead of --files. Also, additional line-start and line-end regexp patterns.

    One change for --limit-files and one for the additional patterns.

@fleimgruber
Copy link
Contributor Author

Thanks for review. I addressed your comments, changed the commit message (typos) and reverted the regexp pattern change. With your consent I would squash.

@Makman2
Copy link
Member

Makman2 commented May 31, 2018

Alright you can squash ;) Please rectify your final commit message so things like line-end aren't mentioned any more (due to your "revert commit" :) )

Uses `--limit-files` instead of `--files`.  coafile: Ignore for gitmate
CI and include for ert.
@fleimgruber
Copy link
Contributor Author

@Makman2 Squashed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants