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

Support for passing gofmt options #34

Open
jdechicchis opened this issue Jan 14, 2021 · 9 comments
Open

Support for passing gofmt options #34

jdechicchis opened this issue Jan 14, 2021 · 9 comments

Comments

@jdechicchis
Copy link

It would be great to have the ability to pass gofmt options such as -s etc. Looking at https://golang.org/src/cmd/gofmt/ not sure what the best approach would be here. I see goimports-reviser is currently using go/format. Ideally, goimports-reviser wouldn't have to duplicate the work of gofmt, including it's flags, but I also see how calling the gofmt binary may not be desired as well. Happy to make a PR, but would appreciate some feedback since I'm not very familiar with this.

@incu6us
Copy link
Owner

incu6us commented Jan 21, 2021

Hi @jdechicchis,
Actually goimports-reviser is created for imports(but we can expand functionality) and yes, it uses go/format to make it possible to perform basic cleanup for the code using public functionality of the library. gofmt encapsulates their methods for -s option and also has BSD license with which we can't just use the implementation. Include the gofmt as a binary into the project is also not a good solution, I believe. I propose you create a list of the functionality to perform cleanup and simplicity for the code and start the discussion in this direction. So, what should be done?

@jdechicchis
Copy link
Author

I see, that makes sense. The licensing is a hurdle. We were thinking that since goimports-reviser does some basic formatting, adding functionality to totally replace the need to call gofmt would be nice. I'd be happy to iron out the requirements more and dive into the code but it's also up to you if you'd want formatting functionality in goimports-reviser which would have to be maintained independently of gofmt going forward.

P.S. the actual import sorting functionality is great :)

@incu6us
Copy link
Owner

incu6us commented Jan 27, 2021

@jdechicchis Thanks:)

Ok. Let's discuss the requirements. Here are some ideas which could be start point:

  • remove empty imports block (for today the tool can't remove code block with empty imports like import());
  • add a new line between funcs. Basicaly, code below
         func...
         func...
    
    should be like
        func...
    
        func...
    

If you have an idea, then welcome:)
Let's discuss them and convert then into tickets/issues

@jdechicchis
Copy link
Author

Apologies for the delayed reply. Those make sense to me. Perhaps we can also look to gofmt's simplify option as a starting point? Even though we can't copy their implementation, I think mirroring the functionality (although someone tedious) may make for a nice addition to goimports-reviser. From their docs:

An array, slice, or map composite literal of the form:
	[]T{T{}, T{}}
will be simplified to:
	[]T{{}, {}}

A slice expression of the form:
	s[a:len(s)]
will be simplified to:
	s[a:]

A range of the form:
	for x, _ = range v {...}
will be simplified to:
	for x = range v {...}

A range of the form:
	for _ = range v {...}
will be simplified to:
	for range v {...}

I suppose one could add a -format option to goimports-reviser to tune the additional formatting with the goal of mirroring (and maybe exceeding!) what gofmt does.

@incu6us
Copy link
Owner

incu6us commented Feb 5, 2021

@jdechicchis Cool, I like it! I'll create separate issues for that(perhaps today-tomorrow) with the label need help and we could start with the implementation. This ticket still be opened to discussing general questions for -format option.

@incu6us incu6us added this to the formating option milestone Feb 5, 2021
@incu6us
Copy link
Owner

incu6us commented Feb 5, 2021

@jdechicchis I've created issues about formating option #36, #37, #38, #39 and #40(issue #35 is a general issue and attached to separated milestone). So, you can pick any of them.

@jdechicchis
Copy link
Author

Sounds great @incu6us! I'm hoping to pick up some of these issues starting next week

@incu6us
Copy link
Owner

incu6us commented Apr 15, 2021

#35 - done

@incu6us
Copy link
Owner

incu6us commented Apr 26, 2021

#36 - done

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

No branches or pull requests

2 participants