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

Remove --strict option from LintPlugin so that it doesn't constantly crash. #489

Closed
wants to merge 4 commits into from

Conversation

macshome
Copy link

@macshome macshome commented Mar 6, 2023

The LintPlugin command plugin has default settings that include --strict in the arguments. This causes any lint run to exit with a 1 and report as an error.

I removed the default inclusion of --strict and added a filter to make sure that it isn't passed in a plugin run.

No tests were added because SPM Plugin targets don't currently support tests.

…e sure that `strict` does not exist in linter plugin args.
Make sure that `strict` does not exist in linter plugin args.
Removed print debugging.
Conflicts:
	Plugins/LintPlugin/plugin.swift
@allevato
Copy link
Member

allevato commented Mar 6, 2023

Would this cause problems for anyone who wanted to use LintPlugin in a CI scenario, where they use SPM to kick off the plug-in and detect success/failure based on the error code?

I don't have a lot of experience with SPM plug-ins in general (and from what I've seen, the design of the overall system is a bit limited), but I'd like to better understand the problem that this is solving and ensure that we're not harming other workflows if we do this.

@macshome
Copy link
Author

macshome commented Mar 6, 2023

The issue is that in the regular operation of the linter if it finds any issue it prints warnings. Those warnings make it exit with a non-zero and then your IDE shows a failure when the plugin finishes.

Here is an example of a successful use of the current command plugin in Xcode.

Screenshot 2023-03-06 at 2 44 09 PM

A simple change would be to not add --strict by default, but also allow it to be added. Simple to do.

I also think though that most folks on CI will just run swift-format lint rather than trying to mess with the command plugin as it is more for interactive use. Unlike a build plugin, the messages here are not in the build logs. Honestly I just run swift-lint lint via a script step in my builds for linting so that I can see the messages in Xcode's code editor.

@allevato
Copy link
Member

allevato commented Mar 6, 2023

If we're offering SPM plug-ins, I don't think we should necessarily assume that people would run swift-format manually in CI use cases. It seems like running the SPM command would be a perfectly reasonable entry point, interactive or not.

If the issue is that Xcode renders this incorrectly, we might have other options here since the Xcode plug-in interface is different than the command-line interface. What if the implementation was updated so that Xcode's plug-in used Diagnostics.warning instead of Diagnostics.error for the failure case? Would that meet your needs without removing --strict?

Longer term, I wonder if we should provide a structured output format for the lint findings (something I've considered before but never had the need to implement), and the Xcode plug-in could capture and parse those and turn each finding individually into a Diagnostics.warning call instead of printing the output to the terminal.

@macshome
Copy link
Author

macshome commented Mar 7, 2023

OK, changing that exit to Diagnostic.warning works well. I'll see what we would need to do in order to just have the Xcode plugin runs use it so that CLI users can still get a failure code when finding linter warnings.

Should I close this PR and make a new one with a more accurate title when it is ready?

@macshome
Copy link
Author

macshome commented Mar 7, 2023

Longer term for Xcode, and other IDEs, need a build plugin so that the warnings appear inline with the code.

With the command plugin you see this in Xcode.
Screenshot 2023-03-07 at 9 19 54 AM

When running swift-lint as part of the build you have far more actionable warnings like this.
Screenshot 2023-03-07 at 9 22 02 AM

I know which output I would rather use.

@macshome
Copy link
Author

macshome commented Mar 8, 2023

Based on discussion above, closing to solve a different way.

@macshome macshome closed this Mar 8, 2023
@allevato
Copy link
Member

allevato commented Mar 8, 2023

OK, changing that exit to Diagnostic.warning works well. I'll see what we would need to do in order to just have the Xcode plugin runs use it so that CLI users can still get a failure code when finding linter warnings.

Should I close this PR and make a new one with a more accurate title when it is ready?

You could have just re-titled the PR and updated the commit messages to reflect a new state of things, but since you've already closed this one, opening a new one is fine too.

Longer term for Xcode, and other IDEs, need a build plugin so that the warnings appear inline with the code.

Is it really the case that Xcode only shows warnings/errors in the source editors for build plugins, not other plugins? That would be unfortunate, because linting doesn't feel like a true "build" plugin, as it's not generating any outputs. SE-0303 seems like its intent is for build plugins to contribute new files to the build, whereas SE-0332 that introduced command plugins uses linting/formatting as its use cases.

I'll have to do some more investigation here.

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.

None yet

2 participants