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

Swiftlint only expects files with type ".swift" #1721

Closed
lswith opened this Issue Jul 28, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@lswith

lswith commented Jul 28, 2017

I know that swiftlint explicitly expects files of type .swift, but I'd like to pass a file with ".lint-test" instead of ".swift". This is mainly for easy integration with https://github.com/phacility/arcanist.

@SDGGiesbrecht

This comment has been minimized.

Show comment
Hide comment
@SDGGiesbrecht

SDGGiesbrecht Jul 28, 2017

Contributor

I’ve seen several issues where quick bug demonstrations were written as scripts like this:

echo "func doSomething() {}" | swiftlint lint --use-stdin

Does it work to pipe your file to swiftlint like that?

Contributor

SDGGiesbrecht commented Jul 28, 2017

I’ve seen several issues where quick bug demonstrations were written as scripts like this:

echo "func doSomething() {}" | swiftlint lint --use-stdin

Does it work to pipe your file to swiftlint like that?

@lswith

This comment has been minimized.

Show comment
Hide comment
@lswith

lswith Jul 28, 2017

lswith commented Jul 28, 2017

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Jul 28, 2017

Collaborator

I don't think we should change the default behavior, but I think it'd be reasonable to offer a way to override it, perhaps in the configuration file.

Collaborator

marcelofabri commented Jul 28, 2017

I don't think we should change the default behavior, but I think it'd be reasonable to offer a way to override it, perhaps in the configuration file.

@lswith

This comment has been minimized.

Show comment
Hide comment
@lswith

lswith Jul 28, 2017

lswith commented Jul 28, 2017

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Jul 28, 2017

Collaborator

Checking if
something is a swift file or not shouldn't be the job of the linter.

I disagree. I think the responsibility of a tool is to provide sensitive defaults. I wouldn't want that, by default, SwiftLint linted Objective-C files for example. Or that we needed to use --path all the times.

A good example is to simply pass globs to the --path variable. Something like --path **/*.swift.

That could also work, but currently I don't think we even support passing a non-Swift file to --path.

Collaborator

marcelofabri commented Jul 28, 2017

Checking if
something is a swift file or not shouldn't be the job of the linter.

I disagree. I think the responsibility of a tool is to provide sensitive defaults. I wouldn't want that, by default, SwiftLint linted Objective-C files for example. Or that we needed to use --path all the times.

A good example is to simply pass globs to the --path variable. Something like --path **/*.swift.

That could also work, but currently I don't think we even support passing a non-Swift file to --path.

@lswith

This comment has been minimized.

Show comment
Hide comment
@lswith

lswith Jul 28, 2017

lswith commented Jul 28, 2017

@SDGGiesbrecht

This comment has been minimized.

Show comment
Hide comment
@SDGGiesbrecht

SDGGiesbrecht Jul 28, 2017

Contributor

@lswith, @marcelofabri

The standard way is to create the .lint-test file.

I do not understand what a .lint-test file is. I ran a Google search for ".lint-test", but it generated no accurate hits. What standard are you referring to?

I added arcanist to the search term list and about the 60th hit was the source code for the unit tests for Arcanist itself.

Those files were the equivalent of SwiftLint’s RuleDescription.(non)TriggeringExamples, and the files (the one’s I saw described CSS) are not pure source, but are repeatedly interrupted with error expectations (analogous to in triggeringExamples) and end with a JSON configuration to use for the unit test.

.lint-test looks to me like an internal implementation detail of Arcanist’s unit tests.

If the .lint-test files I saw are what we are talking about, then SwiftLint is saving you a headache by refusing to lint them, because SourceKit cannot not even parse them (and in my experience SourceKit does not always fail gracefully).


In case this is all a misunderstanding, you do know the normal way to use SwiftLint is to run swiftlint lint in the project root, which outputs a list of violations for every Swift file in the project, right?

Contributor

SDGGiesbrecht commented Jul 28, 2017

@lswith, @marcelofabri

The standard way is to create the .lint-test file.

I do not understand what a .lint-test file is. I ran a Google search for ".lint-test", but it generated no accurate hits. What standard are you referring to?

I added arcanist to the search term list and about the 60th hit was the source code for the unit tests for Arcanist itself.

Those files were the equivalent of SwiftLint’s RuleDescription.(non)TriggeringExamples, and the files (the one’s I saw described CSS) are not pure source, but are repeatedly interrupted with error expectations (analogous to in triggeringExamples) and end with a JSON configuration to use for the unit test.

.lint-test looks to me like an internal implementation detail of Arcanist’s unit tests.

If the .lint-test files I saw are what we are talking about, then SwiftLint is saving you a headache by refusing to lint them, because SourceKit cannot not even parse them (and in my experience SourceKit does not always fail gracefully).


In case this is all a misunderstanding, you do know the normal way to use SwiftLint is to run swiftlint lint in the project root, which outputs a list of violations for every Swift file in the project, right?

@lswith

This comment has been minimized.

Show comment
Hide comment
@lswith

lswith Jul 31, 2017

ok I'll try an explain better. The arcanist test loader (for writing arcanist integration with swiftlint I might add) expects the test files to be ".lint-test" extension. This file is then separated into 2 parts, one which is the source and one which is the lint error. It's basically a generic way to test all linters effectively. Most linters allow the ability to specify a path and then to take any file at all. If no path is specified, then you can have sensible defaults such as linting the whole directory etc.

There are 2 options, I can either change our generic test loader to make accomodations for swiftlint, which is the only linter which ignores a file even when explicitly pointed to. Simply allowing the --path variable to allow for non-swift files would save me in this circumstance and would help put this linter in line with a whole heap of others out there.

lswith commented Jul 31, 2017

ok I'll try an explain better. The arcanist test loader (for writing arcanist integration with swiftlint I might add) expects the test files to be ".lint-test" extension. This file is then separated into 2 parts, one which is the source and one which is the lint error. It's basically a generic way to test all linters effectively. Most linters allow the ability to specify a path and then to take any file at all. If no path is specified, then you can have sensible defaults such as linting the whole directory etc.

There are 2 options, I can either change our generic test loader to make accomodations for swiftlint, which is the only linter which ignores a file even when explicitly pointed to. Simply allowing the --path variable to allow for non-swift files would save me in this circumstance and would help put this linter in line with a whole heap of others out there.

@SDGGiesbrecht

This comment has been minimized.

Show comment
Hide comment
@SDGGiesbrecht

SDGGiesbrecht Jul 31, 2017

Contributor

So then the first and last lines here are running afoul of SwiftLint?

I think it would be wiser if Arcanist’s test loader were to consistently append a meaningful file extension when it creates the temporary files to begin with, but that is just how I would do things.

However, if you just want to get it working easily, I think you could just change line 67 from...

$basename = basename($file);

...to...

$basename = basename($file, ".lint-test");

...based on this example.

Then you just name your Swift test cases according to the pattern MyTestCase.swift.lint-test.

It looks to me like your existing test loader would then spit out temporary files with the .swift extension, and your other linters—which you say are agnostic about file extensions—should not care that the .lint-test extension has been removed from their temporary files.

Contributor

SDGGiesbrecht commented Jul 31, 2017

So then the first and last lines here are running afoul of SwiftLint?

I think it would be wiser if Arcanist’s test loader were to consistently append a meaningful file extension when it creates the temporary files to begin with, but that is just how I would do things.

However, if you just want to get it working easily, I think you could just change line 67 from...

$basename = basename($file);

...to...

$basename = basename($file, ".lint-test");

...based on this example.

Then you just name your Swift test cases according to the pattern MyTestCase.swift.lint-test.

It looks to me like your existing test loader would then spit out temporary files with the .swift extension, and your other linters—which you say are agnostic about file extensions—should not care that the .lint-test extension has been removed from their temporary files.

@lswith

This comment has been minimized.

Show comment
Hide comment
@lswith

lswith Aug 3, 2017

I'll see what the arcanist maintainers say but its just surprising that swiftlint doesn't support exact file names. Ktlint, eslint, terraform lint, flake8, golint, pylint, rubylint, pep8, csslint, coffeelint, puppetlint, and rubocop all seem to be able to either specify the extension of the files they are looking for with sensible defaults or allow a direct file to be passed in.

lswith commented Aug 3, 2017

I'll see what the arcanist maintainers say but its just surprising that swiftlint doesn't support exact file names. Ktlint, eslint, terraform lint, flake8, golint, pylint, rubylint, pep8, csslint, coffeelint, puppetlint, and rubocop all seem to be able to either specify the extension of the files they are looking for with sensible defaults or allow a direct file to be passed in.

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim Aug 4, 2017

Collaborator

I really don't see why we wouldn't make SwiftLint work when passed single files to swiftlint lint --path not_a_swift_file.txt.

Collaborator

jpsim commented Aug 4, 2017

I really don't see why we wouldn't make SwiftLint work when passed single files to swiftlint lint --path not_a_swift_file.txt.

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Aug 5, 2017

Collaborator

We definitely should change that check from isSwiftFile to just check if it's a file. 👍

Collaborator

marcelofabri commented Aug 5, 2017

We definitely should change that check from isSwiftFile to just check if it's a file. 👍

marcelofabri added a commit that referenced this issue Aug 7, 2017

marcelofabri added a commit that referenced this issue Aug 7, 2017

marcelofabri added a commit that referenced this issue Aug 7, 2017

marcelofabri added a commit that referenced this issue Aug 17, 2017

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