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

Remote configs #93

Closed
wants to merge 11 commits into from
Closed

Conversation

orsinium
Copy link
Contributor

Allow -rules option to be a remote URL. This makes ruleguard suitable to be used as a zero-configuration linter.

[x] Tests included. They check a small file with a local and remote copy of rules.go. Later, this file should be duplicated to make it possible to test new checks that weren't introduced in the master branch yet.
[x] README updated.

Minor changes:

  1. Remove redundant if flagRules == "". The if body statement is unreachable.
  2. Parse source code of rules.go right from the stream without reading it from the stream and then converting it into a stream again. It makes file reading a bit faster and memory-friendly.
  3. Provide a short list of ready to use open-source rules.

Copy link
Collaborator

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I left few comments. By the way, the idea of the remote configs(rules) was sliiiiightly another, however fetching them via http should work too.

README.md Outdated Show resolved Hide resolved
@@ -109,10 +126,10 @@ example.go:5:10: !(v1 != v2)

It automatically inserts `Report("$$")` into the specified pattern.

## How does it work?
## How it works
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My linter told me to do so. Namely, it is MD026 check. My motivation is that this is not an FAQ section, so you should answer questions, not to ask them. Also, Betteridge's law of headlines is a bit related as an example of why you should avoid question mark in headlines.

And as the last piece of motivation let's use crowd intelligence:

Copy link
Collaborator

Choose a reason for hiding this comment

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

lol, interesting

analyzer/analyzer.go Outdated Show resolved Hide resolved
analyzer/analyzer_test.go Outdated Show resolved Hide resolved
@@ -0,0 +1,17 @@
// This package tests rules specified in `rules.go` of ruleguard itself.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we reuse another rules and not create new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? This file exists because there are no tests for the root rules.go yet. However, it's good to test them.

```


Create a test `example.go` target file:
Copy link
Collaborator

Choose a reason for hiding this comment

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

what was the point to change this part of the README? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not changed but rearranged. The text remains the same, git is bad at showing moved things. The motivation is to simplify the quick start.

Before:

  • Quick start
    • create example code
    • create example rules
    • run ruleguard using the created rules file

Now:

  • Quick start
    • create example code
    • run ruleguard using remote rules
  • Using custom rules
    • create example rules
    • run ruleguard using local rules

@orsinium
Copy link
Contributor Author

By the way, the idea of the remote configs(rules) was sliiiiightly another

Do you mean distributing plugins as go packages? Yes, @quasilyte told me about it a bit, this is another good idea and it would be cool to have it eventually implemented. However, this network-based implementation has the advantage of zero-configuration allowing a faster start and easier reusing of rules between different projects. This is how shared configs are implemented in flakehell, for instance.

@cristaloleg
Copy link
Collaborator

I'm skeptical of raw http configs.

@orsinium
Copy link
Contributor Author

💔

@orsinium
Copy link
Contributor Author

In a long run, it's always better to keep all the checks in your environment, sure. However, remote configs support opens a door for a faster start in both using ruleguard as a linter and distributing your own rules.

Also, it solves the licensing question. Since go mod has no separation between "prod" and "dev" environments, everything in your go.sum considered your project dependencies. And for project dependencies, you can have license constraints, especially when it is about a proprietary product. So, if rules are distributed under GPL 3.0 to limit their usage in a proprietary service, it could cause issues if using them as a package in you go.mod for purpose of using on dev environment only. This PR implements another approach, that more likely can be interpreted as dynamic linking. And as for dynamic linking, we do it explicitly only on a specified environment, keeping the project dependencies free from questionable licenses.

@cristaloleg
Copy link
Collaborator

remote configs support opens a door for a faster start

via DSL it will be almost the same, but instead of a url, there will be an import.

Regarding go.mod and licensing it's an open question. That's how ecosystem is built. I'm not 100% sure what is the proper answer. But merging http configs without previous discussion is kinda risky. Adding feature and than dropping it is a not proper way.

@orsinium
Copy link
Contributor Author

orsinium commented Oct 23, 2020

All review comments are addressed.

@orsinium
Copy link
Contributor Author

Feel free to re-open if you change your mind

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