checks: add redundant target platform check #5091
Conversation
jsternberg
force-pushed
the
lint-redundant-target-platform
branch
2 times, most recently
from
72cf668
to
5b906e9
Compare
| @@ -316,6 +316,10 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS | |||
| } | |||
|
|
|||
| if v := st.Platform; v != "" { | |||
| // Mark whether the specified platform is redudant for the linter | |||
There was a problem hiding this comment.
Removing this line due to another section of the code review.
| @@ -316,6 +316,10 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS | |||
| } | |||
|
|
|||
| if v := st.Platform; v != "" { | |||
| // Mark whether the specified platform is redudant for the linter | |||
| // to pick up later. | |||
| ds.platformRedundant = v == "$TARGETPLATFORM" | |||
There was a problem hiding this comment.
This can also be "${}". Probably better to check the platMatch, so that the return value equals the targetplatform value and platMatch.Matched equals TARGETPLATFORM.
There was a problem hiding this comment.
I've modified this to use the platform match and integrated it more closely with the existing report unused args so I've removed this variable and this check.
| // Mark whether the specified platform is redudant for the linter | ||
| // to pick up later. | ||
| ds.platformRedundant = v == "$TARGETPLATFORM" | ||
|
|
There was a problem hiding this comment.
If we are going to defer returning the warning to dispatch() (to only return warnings for reachable stages) then it should be done for this reportUnusedFromArgs condition as well. In that case the key in the dispatchState can be something like stageWarnings or warnings and contain the slice of warnings that will all be reported if stage is found to be reachable.
There was a problem hiding this comment.
I've integrated this with the existing report unused args using the same shell match and reported the error there. If we decide that warnings should only be reported if reachable, we can change that for all of them at once. I'd like to avoid modifying how the other variables are reported within this PR.
jsternberg
force-pushed
the
lint-redundant-target-platform
branch
6 times, most recently
from
086fc9e
to
8582d58
Compare
thompson-shaun
changed the title
jsternberg
force-pushed
the
lint-redundant-target-platform
branch
3 times, most recently
from
14b3151
to
eb1e97d
Compare
| // Only match this rule if there was only one matched name. | ||
| // It's psosible there were multiple args and that one of them expanded to an empty | ||
| // string and we don't want to report a warning when that happens. | ||
| if len(nameMatch.Matched) == 1 { |
There was a problem hiding this comment.
Also check len(Unmatched)==0
| RUN apk add --no-cache git | ||
| ``` | ||
|
|
||
| ✅ Good: an alternative `--platform` can be set to compile for the host platform. |
There was a problem hiding this comment.
The main good example should be just skipping the flag.
| // Mark whether the specified platform is redudant for the linter | ||
| // to pick up later. | ||
| ds.platformRedundant = v == "$TARGETPLATFORM" | ||
|
|
jsternberg
force-pushed
the
lint-redundant-target-platform
branch
from
eb1e97d
to
e45eeda
Compare
Adds a linter check that checks if `--platform=$TARGETPLATFORM` is used and sends a linter message marking this as redundant. This is because `$TARGETPLATFORM` is the default. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
force-pushed
the
lint-redundant-target-platform
branch
from
e45eeda
to
fe6a7dc
Compare
Adds a linter check that checks if
--platform=$TARGETPLATFORMis used and sends a linter message marking this as redundant. This is because$TARGETPLATFORMis the default.