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

fill importpath from mod info #11

Merged
merged 1 commit into from
May 13, 2022
Merged

Conversation

sio4
Copy link
Member

@sio4 sio4 commented May 9, 2022

When here runs against the go package but there are no go files in the root, here should provide valid package information but currently Info.ImportPath is not set even though Info.Module is filled by go.mod.

I think this could be the right direction but not fully sure since there are not enough code comments (even for the exported things) and also there is no design document.

This fix will help to resolve cli issue gobuffalo/cli#142

@sio4 sio4 added the enhancement New feature or request label May 9, 2022
@sio4 sio4 requested a review from a team May 9, 2022 16:24
@sio4 sio4 self-assigned this May 9, 2022
@sio4
Copy link
Member Author

sio4 commented May 10, 2022

@paganotoni @fasmat I think this could be fine but I want to hear your opinion, or if I missed something.

@paganotoni
Copy link
Member

IMO this is one of those things we should simplify to be an internal packages within the CLI. I say this because in the context where this was written initially we needed to support GOPATH and modules. Buffalo lives a different reality at this point in which we don't support GOPATH mode, and in that sense we might be carrying a lot of technical debt by keeping using packages like this one.

@sio4
Copy link
Member Author

sio4 commented May 10, 2022

I agree. Some of our toolings are somewhat complex and I hope to make them simple. However, this is about grift which is the core of buffalo task and currently it was broken. (from version 0.18.4, buffalo task is no longer working and it was already released to users)

What do you think? Can we make buffalo work and improve/simplify the structure?

Additionally, I thought bringing markbates/grift into gobuffalo/grift could be one parallel option if we will not throw it away soon.

@sio4
Copy link
Member Author

sio4 commented May 10, 2022

Also dropping GOPATH part from here could be a good thing too.

At the same time, I found a lot of packages indirectly depend on here too. I checked some of them, they depend on here via pkger or some other buffalo-related packages. I think we can keep them using it as v0 or v1, and we can go forward as v2.

@sio4 sio4 force-pushed the fill-importpath-from-mod-info branch from 8ba7273 to 7a768a1 Compare May 13, 2022 10:34
@sio4 sio4 requested a review from paganotoni May 13, 2022 10:43
@sio4
Copy link
Member Author

sio4 commented May 13, 2022

@paganotoni Could you please take a look at this issue again? currently buffalo task is broken in the cli version 0.18.4 and this PR will be the start point of the fix. If you think of another approach, please let me know. I would like to fix the issue as soon as possible since the issue could break buffalo users' workflows.

@paganotoni paganotoni merged commit 301a375 into main May 13, 2022
@paganotoni paganotoni deleted the fill-importpath-from-mod-info branch May 13, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants