-
Notifications
You must be signed in to change notification settings - Fork 1
Updated Linter #1
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
base: master
Are you sure you want to change the base?
Conversation
schuylr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR. I definitely made my fork for personal and company internal use, but I'm happy to accept these code changes if it can still fit our use-cases. I've added comments accordingly.
|
|
||
| def cmd(self): | ||
| """Construct a cmd to provide a relative path to 'codeclimate analyze'.""" | ||
| result = ['codeclimate', 'analyze', '-e', 'structure', '-e', 'duplication'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My fork is purpose-built to only run the structure and duplication engines.
Maybe it would be beneficial to accept some user settings that can set these flags instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why the flags are defined here is because CodeClimate will run extra engines if they're defined in .codeclimate.yml - which I don't need them to in my local environment because I have other linters for them (e.g. Rubocop)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @schuylr
Thank you very much for your response.
As far as I can see in my tests, and I experienced the behavior of SublimeLinter, arguments take place in the SublimeLinter settings. They will be applied either of global settings or project-specific settings. I've tested both. If you set for instance
"linters": {
"codeclimate": {
"args": [
"-e",
"structure",
"-e",
"duplication"
]
}
}in the global settings, codeclimate will ignore the .codeclimate.yml in the project root because it will be executed with arguments ($ codeclimate analyze path/to/file.ext -e structure -e duplication)
I suggest, to take this way of using the SublimeLinter settings as it is designed, instead of "hard coding" the arguments.
linter.py
Outdated
| persist.debug(result) | ||
| return result | ||
| """Set working directory and run 'codeclimate analyze'.""" | ||
| if self.context.get('project_root') is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this new code properly read the .codeclimate.yml file found at the project root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course.
The defaults are initially still set to ${project} (linter.py#L23).
If the project has the type of None and no project is configured, I try to find the path of the first opened folder in the folder pane (linter.py#L52-#L53). If no folder is open in the folder pane, the folder of the currently open file taking place (linter.py#L54-#L55).
As far as I know, the Codeclimate CLI has no argument to set a path to a .codeclimate.yml file. Instead, it expects such one in the working directory of execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand you did your changes for your personal/individual use. However, your changes helped me a lot. I do not "expect" that you will use my changes. Instead, I hope you can give it a try/test it. It would enable me to collect outside feedback to prevent mistakes I didn't/can't discover in my development environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I go to package control directly, I will try bring my changes back to the codeclimate repository in a pull request. Is it okey for you, when I mention you in the copyrights?
# Updated by Schuyler Jager & Andreas for SublimeLinter 4
# Copyright (c) 2020 Schuyler Jager & Andreas
# License: MITReview fixes
Review fixes
Review fixes
Review fixes
Hi @schuylr
Thank you very much for your pull request (qltysh-archive#2) at the codeclimate repository. As far as I can see, the repository is indeed abandoned, as you already mentioned.
I've updated the linter class to get rid of the "file_on_disk" warning and the attribute error of "persist.debug(result)"
"file_on_disk" warning:
attribute error of "persist.debug(result)":
Further on – as far as I understand the
codeclimate cli, it is not necessary to set the "structure" and "duplication" in'codeclimate', 'analyze', '-e', 'structure', '-e', 'duplication'. Thecodeclimeate cliruns these two by default when no.codeclimate.ymlconfiguration is given, e.g. for linter.py:I plan to bring the updated linter to package control. However, it is my first SublimeLinter implementation. I wonder what you think? Does the linter work as expected with my changes, or do you notice regressions?
I would be pleased to hear from you.
PS: My next steps are preparing a new travis ci and do a PR to the SublimeLinter repo.