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

Executer for lint using dotnet format #13

Closed
AgentEnder opened this issue Mar 17, 2021 · 12 comments · Fixed by #49
Closed

Executer for lint using dotnet format #13

AgentEnder opened this issue Mar 17, 2021 · 12 comments · Fixed by #49
Assignees
Labels

Comments

@AgentEnder
Copy link
Member

https://github.com/dotnet/format

@AgentEnder AgentEnder added enhancement New feature or request stretch-goal labels Mar 17, 2021
@bcallaghan-et
Copy link
Collaborator

For this and other tool-based features, we'll need a way to manage .NET tools. The two ways we could install a tool are globally and locally. I prefer locally as this matches npm devDependencies. To install and use a local tool, the plugin would need to

  1. Create a tool manifest (if one does not exist) using dotnet new tool-manifest
  2. Install the required tool locally (if it is not already installed) using dotnet tool install dotnet-format
  3. Use the locally installed tool with dotnet format <options>.

Other developers using the same repository will not need to install the tool again, but they will need to run dotnet tool restore, similar to a NuGet package restore, before they can use the tool. This could be handled by the generator each time, or a separate script could be added. I like the idea of running dotnet restore and dotnet tool restore during the npm install sequence, but that probably warrants its own feature request.

Next, we would need to handle updating the tool as it publishes new releases, as this plugin publishes new releases, or both. It's also possible that we would need to handle uninstalling the tool, if this plugin were ever uninstalled.

@AgentEnder Any guidance on how you would architect this solution?

@AgentEnder
Copy link
Member Author

I definitely agree about installing tools being the path for getting this executor up and running.

I see something like this:

  1. Init schematic is updated to create the tool-manifest, with some messaging about "Tool Manifest created for managing dotnet tools"
  2. Tools are installed if needed at the time a schematic that would use them is used the first time.
    • In practice, this means that we can have this format tool, or the swagger cli tool later, or other tools be known by the package, but only installed if users care about them.
    • A process flow with this would look something like: User runs nx format my-api -> Executor checks for tool in manifest, installs if missing -> Executor checks that tool has been restored, reports error message otherwise -> Executor runs tool
  3. Add a restore schematic (simple schematic, runs dotnet restore and dotnet tool restore)
  4. Update init schematic to add nx g @nx-dotnet/core:restore to npm prepare scripts. Something like:
        const package = readJson(host, 'package.json')
        const prepareSteps = package.scripts.prepare?.split?.('&&')?.map?.(x => x.trim()) ?? [];
        if (!prepareSteps.includes('nx g @nx-dotnet/core:restore')) {
            prepareSteps.push('nx g @nx-dotnet/core:restore')
        }
        package.scripts.prepare = prepareSteps.join(' && ')
        writeJson(host, 'package.json', package)

I'll create new issues for the tool management when I am home this evening.

After we get tool-mgmt in, this executor should be pretty quick to test.

@AgentEnder
Copy link
Member Author

With #44 closed, if anyone wants to take this feel free. The main things to remember is that we should support all options that dotnet format does.

I would want --check to be enabled by default for this executor, as it is a linter. The schema file should still contain check as an option, but should also contain an option fix that would disable check. This part is just to maintain consistency with nx lint ... --fix that people are used to using.

@bcallaghan-et
Copy link
Collaborator

I can work on this today. How do you want to handle the list-style options (diagnostics, include, exclude)? Should the schema allow for an array, or should the executor expect that the user has already formatted the list appropriately (comma or space separated)?

@AgentEnder
Copy link
Member Author

I don't believe the schema.json files allow for array types (if I am wrong, correct me as this is what would be ideal). Try a comma separated string and check if that works, if that does work we can go forward with that (noting it in the description of the option). If that does not work, we will figure something else out.

@bcallaghan-et
Copy link
Collaborator

JSON Schema files allow for arrays. The better question is if Nx allows for arrays. If we specify an array type in the schema.json, then users will be able to type the value as an array in workspace.json, but they might not be able to use that option through the command line. I don't know how or even if Nx handles list-style options when coercing command line arguments to the JS object received by the executor.

Consider the default Angular builder. It uses an array to specify assets, styles, and scripts that should be bundled with the application, but these options can't be specified by command line. They're only specified in workspace.json.

@AgentEnder
Copy link
Member Author

Type them as arrays than, but for diagnostics in particular I could see someone wanting to run against 1 particular diagnostic as a use case so it may be worth adding a diagnostic parameter as well. This parameter would override the diagnostics array if it is provided.

bcallaghan-et referenced this issue in Etogy/nx-dotnet May 19, 2021
Add a new executor to lint and format projects using an external tool.

Fixes #13
@bcallaghan-et
Copy link
Collaborator

Should the new format executor be added to new projects by default? Should a migration be created to add it to existing projects that use an older version of the plugin?

@AgentEnder
Copy link
Member Author

I would add it to all projects, but call lint instead since that will be the default functionality.

@AgentEnder
Copy link
Member Author

@bcallaghan-et you could have the executor be called format since it is calling dotnet format, but the architect section's key should be lint with the check option enabled.

github-actions bot pushed a commit that referenced this issue May 20, 2021
# [0.7.0](v0.6.2...v0.7.0) (2021-05-20)

### Features

* **core:** add lint config to generated projects ([d320ce8](d320ce8))
* **core:** add migration to add lint target ([e391744](e391744))
* **core:** add new executor for dotnet-format ([92afd05](92afd05)), closes [#13](#13)
@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 0.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants