Extract gomodifytags to a library by madelinekalil · Pull Request #117 · fatih/gomodifytags

madelinekalil · 2025-02-12T15:58:45Z

This PR moves the gomodifytags tool into its package to be imported by other applications (i.e.: gopls).

A new entrypoint into gomodifytags is provided via config.Apply(), which uses the specified start and end positions to apply struct tag modifications to the node without returning the new content.

related: golang/vscode-go#2002

main.go

modifytags/gomodifytags.go

findleyr · 2025-02-19T20:23:57Z

modifytags/gomodifytags.go

+
+	ast.Inspect(node, rewriteFunc)
+
+	c.Start = start


This looks wrong. We should not be mutating the config. Perhaps this is meant to set the output start and end?

Looks like the Start and End values aren't configured by the user, we just store them in the config. They're set by gomodifytags during the findSelection step which uses the line, offset, or struct selection to determine the start and end position of fields that will be modified. (We don't use findSelection in the new Apply routine since we are handling finding the selection on the gopls side).

findleyr · 2025-02-19T20:36:52Z

modifytags/gomodifytags.go

+
+	Fset *token.FileSet
+
+	Remove        []string


I just did an experiment, that I think works out nicely:

There's this tension that many of the fields in the Config are actually related to the set up and tear down of the Run, and are not relevant to Apply. Specifically, if the Run is structured as Parse->Find->Apply->Format, many of these options are related to Parsing, Finding, and Formatting.

I think we should break up these fields into two structs, called something like Pass and Mutation. Pass holds everything above Remove, and Mutation holds everything in Remove and below.

The signature of Apply can then be:
func (m *Mutation) Apply(fset *token.FileSet, node ast.Node, mutation Mutation)

or maybe
func (m *Mutation) Apply(fset *token.FileSet, node ast.Node, start, end token.Pos)

(I'm not sure whether we need to pass start, end in gopls).

In fact, the more I read this, the more I think we should keep the Pass objects in the main package. We can of course move it here later if it is needed, but it is rather specific to the command line, and it seems prudent to keep the new API we are adding as minimal as possible.

Thanks! I moved the Pass fields and methods back into the main package, so now the modifytags package only exposes Apply, Rewrite, Validate, and the Mutation and RewriteErrors types used by the main run() method

modifytags/gomodifytags.go

findleyr

Thanks for your patience! I have some high level comments about the API which, while superficial, I think we should sort out before finishing the review.

README.md

main.go

modifytags/modifytags.go

fatih · 2025-03-05T16:59:56Z

Thanks everyone for the thorough work. I welcome the new changes. On my end, the changes look great and I can merge it whenever you want. Please let me know if this finished and ready to be merged. I can cut a new release afterwards (v1.18.0)

findleyr

This looks great! I mostly commented on comments, with two meaningful exceptions:

I think that Apply should accept a range of byte offsets, not a range of lines, since that is more flexible.
Should we rename Mutation to Modification? I hadn't thought of this name before, and now that I have, it seems obvious.

I've reviewed everything: once we sort out these comments, this is good to go!

main.go

modifytags/modifytags.go

findleyr · 2025-03-10T16:08:14Z

modifytags/modifytags.go

+
+// Apply applies the struct tag modifications of the receiver to all
+// structs contained within the given node between start and end lines, mutating its input.
+func (mut *Mutation) Apply(fset *token.FileSet, node ast.Node, start, end int) error {


Eek, I wonder if we should make this an offset, rather than a line. Sorry for missing this in my review of the API before.

We can keep the command line interface to pass line, but passing an offset is more useful for a tool integration.

For example, if I have struct{ a, b, c int }, and want to add struct tags to a and b, I shouldn't need to split my type expression on to multiple lines.

We decided to pass token.Pos for start and end.

modifytags/modifytags.go

modifytags/modifytags_test.go

modifytags/modifytags.go

main.go

modifytags/modifytags_test.go

findleyr

LGTM modulo the remaining comments, which I think are straightforward! Once these are addressed, let's ask fatih to review.

main.go

findleyr · 2025-03-20T14:59:17Z

main.go

-		structs[x.Pos()] = &structType{
-			name: structName,
-			node: x,
+		structs[x.Pos()] = &ast.TypeSpec{


I'm so sorry, but my understanding of this code was inaccurate, and the suggestion to use a TypeSpec was wong.

I thought we only cared about type declarations, but I see now that the pre-existing logic also handles

var V struct { F int }

That's why we can't simply keep track of type specs. Therefore, I think you should just revert back to the structType type, perhaps with longer documentation explaining that structs can also be selected by their variable name.

Makes sense thanks, I also forgot about handling the variable case.

main.go

modifytags/modifytags.go

modifytags/modifytags_test.go

This PR moves the gomodifytags tool into its package to be imported by other applications (i.e.: gopls). A new entrypoint into gomodifytags is provided via Modification.Apply(), which uses the specified start and end positions to apply struct tag modifications to the file without returning the new content. Also updates staticcheck version related: golang/vscode-go#2002

findleyr · 2025-03-24T13:14:25Z

@fatih I think these are ready for your review.

I suggest we proceed as follows:

Finish this review and merge this PR. Do not tag yet.
@madelinekalil will integrate these into gopls by requiring gomodifytags@master (a pseudoversion).
We review this integration.
Tag the next version gomodifytags.

This may be overly cautious, but it's possible that reviewing the integration into gopls may turn up API considerations that we didn't anticipate in this CL, so if we hold off on tagging a bit, it may be better.

fatih · 2025-03-25T17:15:21Z

Thanks for the great work. I've followed along all the contributions made so far and I think I don't have much to add here. I agree that it makes sense to test the HEAD before cutting a release. I'm merging the PR now, and won't cut a tag until I get a sign off from you folks.

Previously, the VSCode extension installed and executed gomodifytags directly. To reduce third-party dependencies in gopls, we are submitting a change to gomodifytags to extract the functionality into a library called by gopls (fatih/gomodifytags#117). Now, the extension will call the gopls.modify_tags command and the command handler will invoke the modifytags package's Apply() method. Also adds basic code actions for adding and removing struct tags. This will be extended once we implement a dialogue feature in gopls. Change-Id: Idf130c95eec5d469a454cb6f21897629b3364b06 Reviewed-on: https://go-review.googlesource.com/c/tools/+/652495 Reviewed-by: Hongxiang Jiang <hxjiang@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com>

madelinekalil force-pushed the mkalil/create-package branch 2 times, most recently from 677b892 to 95ee7b3 Compare February 19, 2025 16:02

findleyr reviewed Feb 19, 2025

View reviewed changes

madelinekalil force-pushed the mkalil/create-package branch 2 times, most recently from 3085e5f to 8aff27c Compare February 25, 2025 21:03

findleyr reviewed Feb 28, 2025

View reviewed changes

madelinekalil force-pushed the mkalil/create-package branch 2 times, most recently from 0f50ad5 to 1cfbd05 Compare March 5, 2025 16:25

findleyr reviewed Mar 10, 2025

View reviewed changes

madelinekalil force-pushed the mkalil/create-package branch 2 times, most recently from a7689e7 to 3a5d8cf Compare March 13, 2025 20:34

findleyr reviewed Mar 13, 2025

View reviewed changes

modifytags/modifytags.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

main.go Show resolved Hide resolved

madelinekalil force-pushed the mkalil/create-package branch from 3a5d8cf to fba5866 Compare March 14, 2025 16:04

findleyr reviewed Mar 19, 2025

View reviewed changes

madelinekalil force-pushed the mkalil/create-package branch from fba5866 to 1cc6959 Compare March 19, 2025 20:29

findleyr approved these changes Mar 21, 2025

View reviewed changes

madelinekalil force-pushed the mkalil/create-package branch from 1cc6959 to 91c121f Compare March 21, 2025 20:30

fatih merged commit 8c663b1 into fatih:main Mar 25, 2025
1 check passed

fatih mentioned this pull request Mar 25, 2025

Extract gomodifytags into a library #89

Closed

Conversation

madelinekalil commented Feb 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findleyr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fatih commented Mar 5, 2025

Uh oh!

findleyr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findleyr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findleyr commented Mar 24, 2025

Uh oh!