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

Changes to make actionlint usable when using it as a library #327

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

hugo-syn
Copy link
Contributor

@hugo-syn hugo-syn commented Jul 30, 2023

I've added a constructor for RuleBase and changed error and errorf from private to public, otherwise It's not possible to create new rules. I hope that this is the best option, if not, how should we create new rules when using actionlint as a library ?

I also changed wait from private to public in process.go.

@hugo-syn hugo-syn changed the title Changes to make RuleBase usable when using actionlint as a library Changes to make actionlint usable when using it as a library Jul 30, 2023
@rhysd
Copy link
Owner

rhysd commented Jul 31, 2023

What is the reason to make these APIs public? Especially, RuleBase is a base type for existing rules so it is unnecessarily public if you just use the rules. Do you make your own rules? I'd like to confirm your intention before review.

@hugo-syn
Copy link
Contributor Author

Yes I would like to make my own rules

@rhysd
Copy link
Owner

rhysd commented Jul 31, 2023

Sounds nice. However, current API doesn't assume adding new rules. You need to reimplement Linter.check since rule instances are internally created in this method. This is strongly not recommended because it depends on implementation details which can be frequently modified.

Do you want some API to define additional rules?

For example:

opts := &LinterOptions {
    AdditionalRules: func() []Rule {
        return []Rule {
            NewYourOwnRule(),
        }
    }
}

linter = NewLinter(os.Stdout, opts)
linter.LintFiles("/path/to/dir", nil)

process.go Outdated Show resolved Hide resolved
Co-authored-by: Linda_pp <[email protected]>
@hugo-syn
Copy link
Contributor Author

I was actually reimplementing my own Linter to run my custom rules, because I don't want all the rules, so I just need to access the RuleBase and the methods error and errorf to create new rules

@hugo-syn hugo-syn requested a review from rhysd July 31, 2023 15:46
@rhysd
Copy link
Owner

rhysd commented Aug 1, 2023

OK. But please note that you can filter out errors from specific rules by -ignore.

@rhysd rhysd merged commit fbb13d8 into rhysd:main Aug 1, 2023
rhysd added a commit that referenced this pull request Aug 1, 2023
@rhysd
Copy link
Owner

rhysd commented Aug 1, 2023

@hugo-syn I added OnRulesCreated hook API at 655d745. Note that now NewRuleBase returns RuleBase instead of *RuleBase since it is more useful for emebdding.

You can add/remove rules without reimplementing check method.

o := &LinterOptions{
    OnRulesCreated: func(rules []Rule) []Rule {
        // Remove 'runner-label' rule
        for i, r := range rules {
            if r.Name() == "runner-label" {
                rules = append(rules[:i], rules[i+1:]...)
                break
            }
        }
        // Add your own rule
        rules = append(rules, NewYourOwnRule())
        return rules
    },
}

So you no longer need to reimplement Linter.check only for adding/removing rules. With this API, can the ConcurrentProcess API be back to private since you no longer need to call NewRuleShellcheck by yourself?

I'd like to keep the API private since it is an implementation detail and can be updated on patch releases.

@hugo-syn
Copy link
Contributor Author

hugo-syn commented Aug 1, 2023

Sounds nice, I think that I don't need to re-implement my linter now, thanks :)

@rhysd
Copy link
Owner

rhysd commented Aug 2, 2023

Great. Thank you for confirming that. I'll revert ConcurrentProcess to concurrentProcess

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.

2 participants