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

let luacheck find luacheck itself #1057

Merged
merged 1 commit into from Sep 1, 2016

Conversation

Projects
None yet
3 participants
@ghprince
Contributor

ghprince commented Aug 30, 2016

As discussed in #1047, luacheck is capable of finding its config file .luacheckrc itself, it also works better with --filename option.

Therefore, we should remove config-file call for lua-luacheck

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 30, 2016

@ghprince Thanks a lot!

May I ask you to remove the definition of the variable as well, and maybe add a note about the removal to CHANGES.rst?

CHANGES.rst Outdated
@@ -1,6 +1,11 @@
30-cvs (in development)
=======================
- Improvements:

This comment has been minimized.

@lunaryorn

lunaryorn Aug 30, 2016

Contributor

Please list this as **Breaking change**:, as it'll break configurations that try to set this variable explicitly.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 30, 2016

@ghprince Oh, I forgot about doc/languages.rst which is the documentation that appears on our website. Please remove the documentation of the option in this document (that's what's breaking Travis currently).

@ghprince

This comment has been minimized.

Contributor

ghprince commented Aug 30, 2016

@lunaryorn thx a lot for your advice. working on it :) do I need to remove the section in doc/legacy/flycheck.html?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 30, 2016

@ghprince Please ignore that. It's just for reference for stuff that's not in the new manual yet, but it'll go away eventually.

@ghprince

This comment has been minimized.

Contributor

ghprince commented Aug 30, 2016

@lunaryorn also there is a test in test/flycheck-test.el try to use test/resources/language/lua/luacheckrc as config file. After removing those, shall we add a test case to see if luacheck really finds the .luacheckrc as advertised? if yes, I wonder how... creating a file on the fly?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 30, 2016

@ghprince Ah, sorry. I forgot about the test. Just remove it. With the option gone, it's redundant, and I don't think that we should try and test luacheck's behaviour. I think we can just rely upon luacheck doing its job properly, and I hope that luacheck has tests for that 😎

So just remove that test case and the corresponding config file at test/resources/language/lua/luacheckrc.

@ghprince

This comment has been minimized.

Contributor

ghprince commented Aug 30, 2016

@lunaryorn oops, didn't see your reply... I fixed that test by copy-and-then-delete to create temporary .luacheckrc file. It works and passed make LANGUAGE=lua integ on my local machine. I also fixes other broken luacheck integration test cases. Since those broken tests failed on master too, may I ask why they are not running during CI?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 31, 2016

@ghprince Oh, thanks for fixing these test cases. I wished we could run them on Travis CI, and in fact we used to do that long ago but it didn't work so well. Travis CI took ages to install all the syntax checker tools that we support, and tests would constantly break due to minor changes in the output of some tool so we'd spend a non-trivial amount of time on constantly updating the test cases.

It got out of hand and that's where I got fed up, split the integration tests from the rest and decided to stop running the integration tests on Travis CI. Our builds are a lot more stable now, and feedback for contributors comes much faster.

We now only use the integration tests for local testing when making changes to syntax checkers, i.e. I sometimes ask contributors to run make LANGUAGE=foo integ to run the integration tests for a particular language they've been making changes to.

If you've got any idea how we could run the language tests or at least a subset thereof on Travis CI or any other CI platform I'd be happy to hear it.


As to this PR, please rebase on master and resolve the merge conflict in CHANGES.rst. Please put the breaking changes section first, before New syntax checkers.

(flycheck-ert-resource-filename "language/lua/.luacheckrc"))
(flycheck-ert-should-syntax-check
"language/lua/warnings.lua" 'lua-mode)
(delete-file (flycheck-ert-resource-filename "language/lua/.luacheckrc")))

This comment has been minimized.

@lunaryorn

lunaryorn Aug 31, 2016

Contributor

That's a nice idea, but as far as I'm concerned I'd like to ask you to remove the test case entirely. All that we're still testing here is that luacheck itself works correctly and I think that's not our concern. I think we can rely upon luacheck to do its job correctly.

Please remove this test case and the luacheckrc configuration file. Let's keep the tests confined to what's really done by Flycheck, i.e. parsing the output of Luacheck which is already covered by the other test cases for luacheck.

@ghprince ghprince force-pushed the ghprince:let_luacheck_find_luacheckrc branch from 517318e to b79e282 Aug 31, 2016

@ghprince

This comment has been minimized.

Contributor

ghprince commented Aug 31, 2016

@lunaryorn Thanks a lot for your explanation! It's good to know the history. I don't have any ideas of improving the workflow now, but will definitely share with you when I come up with something.

And I have rebased this branch onto current master and fixed the conflict as you suggested.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 31, 2016

@ghprince Thanks a bunch. LGTM now. Would you mind to squash your commits?

@ghprince ghprince force-pushed the ghprince:let_luacheck_find_luacheckrc branch from b79e282 to 709d766 Aug 31, 2016

@ghprince

This comment has been minimized.

Contributor

ghprince commented Aug 31, 2016

@lunaryorn I think github merge button can do squash-and-merge. but anyway I just squashed this branch :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Aug 31, 2016

@ghprince Yup, but just in case someone accidentally presses merge 😉 and in somecases I prefer to merge normally because Github's squash merge looses commit signatures 😞

By the way, would you like to join Flycheck and help maintain Lua support? We've got no Lua team yet and could use some help here 😊 In fact we could use any helping hand, and I'd be happy to welcome you as a contributor 😊

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Aug 31, 2016

LGTM; thanks for this contribution!

@ghprince

This comment has been minimized.

Contributor

ghprince commented Sep 1, 2016

@lunaryorn Thanks a lot for your invitation. I would like to join :)

@lunaryorn lunaryorn merged commit a6d6806 into flycheck:master Sep 1, 2016

3 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

lunaryorn added a commit that referenced this pull request Sep 1, 2016

@lunaryorn lunaryorn referenced this pull request Sep 1, 2016

Merged

Add @ghprince to lua team #1061

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment