realm / SwiftLint

A tool to enforce Swift style and conventions.
https://realm.github.io/SwiftLint
MIT License
18.7k stars 2.23k forks source link

Match Kind FileText for Custom Rules #1495

Closed mohamede1945 closed 7 years ago

mohamede1945 commented 7 years ago

I was trying to add a custom rule that matches a regex of function implementations, but I don't think that exists. Suppose the following file

func foo() {
    expectedToBeImplementedBeforeRelease()
}

I wanted to match all occurrences of expectedToBeImplementedBeforeRelease. Is this currently possible?

nigelflack commented 7 years ago

You could create a new custom rule for that - it looks like a straightforward case. What about it doesn't work?

There's also a TODO rule in the default ruleset. It's worded a little strangely I think as it says "TODOs should not be used" or something like that. But it does highlight their existence. So you could use a TODO comment - and not allow a release if any still exist.

(note I'm not really a major contributor, but I've experimented a bit with custom rules and am getting the hang of them)

mohamede1945 commented 7 years ago

You could create a new custom rule for that - it looks like a straightforward case. What about it doesn't work?

I did, but the custom rule didn't work because it matches only one of the match_kinds explained in the readme and none of them matches the implementation text.

So you could use a TODO comment - and not allow a release if any still exist.

The todo comment purpose is a little bit different than my expectedToBeImplementedBeforeRelease because this function returns Never, so I don't need to provide mock implementation for foo.

Here is how I use it, notice it returns VeryComplexObjectToConstruct

func foo() -> VeryComplexObjectToConstruct {
    expectedToBeImplementedBeforeRelease()
}

If I used Todo, I will need to always keep both statements in sync

func foo() -> VeryComplexObjectToConstruct {
    // TODO: must be implemented
    expectedToBeImplementedBeforeRelease()
}

But that's very specific to my problem. In general, I think it is very good practice to have custom rules to match against the entire text of the file, this way I can write a regex to validate against bad code snippets, etc.

marcelofabri commented 7 years ago

What if you marked expectedToBeImplementedBeforeRelease() as deprecated?

@available(*, deprecated, message: "This should be implemented before releasing!")
func expectedToBeImplementedBeforeRelease() -> Never { }
mohamede1945 commented 7 years ago

@marcelofabri That's a very nice trick. I will use it immediately.

While this satisfies my own specific problem. But again, I think it is very good practice to have custom rules to match against the entire text of the file, this way I can write a regex to validate against bad code snippets, etc.

What do you think?

marcelofabri commented 7 years ago

Couldn't you just avoid using matchKinds?

mohamede1945 commented 7 years ago

Doesn't work

marcelofabri commented 7 years ago

Also, you can see that the method call is a source.lang.swift.syntaxtype.identifier token, so you could use that instead.

mohamede1945 commented 7 years ago

Again, you are trying to solve a specific case. The general case is to have the ability to match Regex against file content.

marcelofabri commented 7 years ago

Works for me:

custom_rules:
  rule_test:
    name: "Rule Test"
    regex: "expectedToBeImplementedBeforeRelease"
$ swiftlint 
Loading configuration from '.swiftlint.yml'
Linting Swift files in current working directory
Linting 'file.swift' (1/1)
/Users/marcelofabri/Documents/file.swift:2:5: warning: Rule Test Violation: Regex matched. (rule_test)
Done linting! Found 1 violation, 0 serious in 1 file.
mohamede1945 commented 7 years ago

Yes, it's working! But when I tried to use it with whitelist_rules it didn't

whitelist_rules:
  - rule_test

custom_rules:
  rule_test:
    name: "Rule Test"
    regex: "expectedToBeImplementedBeforeRelease"
marcelofabri commented 7 years ago

But that's another issue actually. I don't think individual custom rules are supported on any of theses lists (whitelist_rules, disabled_rules, etc).

You probably could use custom_rules to allow all of them though.

mohamede1945 commented 7 years ago

Yeah, that's true. I want all my custom_rules to be working. But when I added whitelist_rules it didn't work. So is it possible to give me a .yml file that has both whitelist_rules and also custom_rules and all rules inside both working?

For example:

whitelist_rules:
  - empty_count

custom_rules:
  rule_test:
    name: "Rule Test"
    regex: "expectedToBeImplementedBeforeRelease"

I expect to have both empty_count and rule_test to work. But it seems rule_test is not validated

marcelofabri commented 7 years ago

I think it should work if you use:

whitelist_rules:
  - empty_count
  - custom_rules

custom_rules:
  rule_test:
    name: "Rule Test"
    regex: "expectedToBeImplementedBeforeRelease"
mohamede1945 commented 7 years ago

Ok, thank you so much!

mohamede1945 commented 7 years ago

That was really helpful

marcelofabri commented 7 years ago

Feel free to open another issue if you have more questions 👍