Skip to content

fix: only-new-issues should return empty string for fetchPatch #600

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

Closed

Conversation

conorevans
Copy link

@conorevans conorevans commented Nov 11, 2022

fetchPatch determines the patchPath. If the patchPath is not empty, the action explicitly overrides user input and sets --new-from-patch=<patch_path>, --new=false and --new-from-rev to be empty. only-new-issues should obviously set --new to be true.

Closes #531

Signed-off-by: Conor Evans [email protected]


This is in fetchPatch. This is then used in prepareEnv

async function prepareEnv(): Promise<Env> {
  // ...
  const patchPromise = fetchPatch()
  // ...
  const patchPath = await patchPromise
  // ....
  return { lintPath, patchPath }
}

prepareEnv is then used to get the arguments for runLint - again we have patchPath that should be returned as "" when only-new-issues is true.

export async function run(): Promise<void> {
// ...
    const { lintPath, patchPath } = await core.group(`prepare environment`, prepareEnv)
// ...
    await core.group(`run golangci-lint`, () => runLint(lintPath, patchPath))

In runLint the patchPath if set explicitly overrides the new arguments...

async function runLint(lintPath: string, patchPath: string): Promise<void> {
  // ...

  if (patchPath) {
    if (userArgNames.has(`new`) || userArgNames.has(`new-from-rev`) || userArgNames.has(`new-from-patch`)) {
      throw new Error(`please, don't specify manually --new* args when requesting only new issues`)
    }
    addedArgs.push(`--new-from-patch=${patchPath}`)

    // Override config values.
    addedArgs.push(`--new=false`)
    addedArgs.push(`--new-from-rev=`)
  }

Pretty self explanatory fix all the same.

@conorevans
Copy link
Author

FYI @ldez @EtienneM - Etienne for the moment if you simply set only-new-issues to false it will act as you think it would when you set true.

@ldez ldez self-requested a review November 12, 2022 03:02
@ldez ldez added the bug Something isn't working label Nov 12, 2022
@EtienneM
Copy link

Hi @ldez 👋

Do you think it could be possible to have a look at this PR?

@ldez ldez force-pushed the conorevans/531-only-new-issues branch from adeb075 to 874b3d9 Compare February 24, 2023 08:18
conorevans and others added 2 commits February 24, 2023 10:07
fetchPatch determines the patchPath. If the patchPath is not empty, the action explicitly overrides user input and sets --new-from-patch=<patch_path>, --new=false and --new-from-rev to be empty. only-new-issues should obviously set --new to be true.

Signed-off-by: Conor Evans <[email protected]>
@ldez ldez force-pushed the conorevans/531-only-new-issues branch from 2cdc670 to 1372a1c Compare February 24, 2023 09:07
@ldez
Copy link
Member

ldez commented Apr 27, 2024

This PR will completely change the behavior:

  • if only-new-issues is true: it will not generate and use the patch from GitHub
  • if only-new-issues is false (the default): it will generate and use the patch from GitHub

This is not the expected behavior.

The assumptions of this PR are wrong.

  • only-new-issues is only here to use the patch generated by GitHub.
  • --new is to check unstaged changes or untracked files or changes in HEAD~
  • only-new-issues is not related to --new

https://golangci-lint.run/usage/configuration/#issues-configuration

@ldez ldez closed this Apr 27, 2024
@ldez ldez added declined and removed bug Something isn't working labels Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

only-new-issues does not seem to work on my setup
3 participants