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

support relative paths in linter + spans that span multiple characters/lines #369

Merged
merged 4 commits into from
Mar 31, 2016

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 30, 2016

previously the lints did not show up in files if the paths were relative. Alternatively this could be changed in atom-linter directly, but I don't think that's feasible as atom-linter doesn't know about projects, but atom-build explicitly does.

fixes #359

@oli-obk oli-obk changed the title support relative paths in linter support relative paths in linter + spans that span multiple characters/lines Mar 30, 2016
this.linter && this.linter.setMessages(this.errorMatcher.getMatches().map(match => ({
type: 'Error',
text: match.message || 'Error from build',
filePath: match.file,
filePath: path.isAbsolute(match.file) ? match.file : path.join(atom.project.getPaths()[0], match.file),
Copy link
Owner

Choose a reason for hiding this comment

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

The error match file/configuration is not necessarily in the first project. The path you're actually looking for here is in the variable p (determined at the top of this function).

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 30, 2016

addressed comments by fixing the appropriate commits

@noseglid
Copy link
Owner

What is the problem with relative paths? It works well for me:
error-match

Possibly it should be relative cwd instead? As that is the path that the error is most likely to be relative to.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 30, 2016

the problem is that you don't get a hit on File only on Project. You can seen this at the bottom of the screen. With the changes implemented in this PR, you also get a squiggly line at the line+col location and a popup message showing the error message (if any). With the changes I made to cargo, you get a squiggly line below the entire error location (this can be applied to gcc errors and many others, too).

@noseglid
Copy link
Owner

Still something weird with the test: https://travis-ci.org/noseglid/atom-build/jobs/119506163

Also, I think it makes more sense to take it relative to the cwd of the executed command (rather than active project path).

@noseglid
Copy link
Owner

This will solve #359

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 31, 2016

Also, I think it makes more sense to take it relative to the cwd of the executed command (rather than active project path).

oh. that makes sense

@noseglid
Copy link
Owner

cwd is available in that scope, unless I am mistaken. So should be as easy as changing that

And the remaining failing spec probably has the same fix as the one you already corrected. (You can run specs locally in your Window: Run Package Specs). They require no setup.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 31, 2016

changed to use cwd and fixed the test

@noseglid
Copy link
Owner

The slashes / in the specs should be backslashes \ on windows. Use path.join to get the correct delimiter.

@oli-obk oli-obk force-pushed the master branch 4 times, most recently from 7aecefa to 3aa1a2d Compare March 31, 2016 13:20
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 31, 2016

travis and appveyor are happy now

@noseglid
Copy link
Owner

Very nice. Thanks for this!

@noseglid noseglid merged commit 21a37ce into noseglid:master Mar 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

errorMatch: option to capture character range
2 participants