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

feat!(v2): use go modules info #94

Merged
merged 8 commits into from
Feb 28, 2022
Merged

feat!(v2): use go modules info #94

merged 8 commits into from
Feb 28, 2022

Conversation

Bobgy
Copy link
Collaborator

@Bobgy Bobgy commented Dec 31, 2021

UPDATE: although this is a breaking change, we decided to release it in v1.1, because we don't believe there are still people not using go modules... That's our assumption, let us know if the new release breaks you!

BREAKING CHANGE: this tool no longer natively supports go projects managed by GOPATH. It only works with go modules projects now.

Changes:

@Bobgy Bobgy force-pushed the v2-csv-1 branch 3 times, most recently from e3fc617 to 30635f9 Compare December 31, 2021 06:16
@Bobgy Bobgy force-pushed the v2-csv-1 branch 4 times, most recently from e06b387 to 6598151 Compare January 5, 2022 07:50
@Bobgy Bobgy changed the title feat(v2): basic csv command using modules feat(v2): csv command using module info Jan 5, 2022
@Bobgy Bobgy force-pushed the v2-csv-1 branch 2 times, most recently from 5cc6d1d to 09e68a1 Compare January 5, 2022 11:48
@Bobgy Bobgy changed the base branch from v2 to master January 14, 2022 01:51
@Bobgy
Copy link
Collaborator Author

Bobgy commented Jan 14, 2022

Updated to point to master branch

Copy link
Contributor

@wlynch wlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments may come once this is rebased 🙏

csv.go Outdated Show resolved Hide resolved
csv.go Show resolved Hide resolved
csv.go Outdated Show resolved Hide resolved
licenses/find.go Show resolved Hide resolved
licenses/git.go Show resolved Hide resolved
licenses/library.go Outdated Show resolved Hide resolved
return "", wrap(err)
}
// TODO: there are still rare cases this may result in an incorrect URL.
// https://github.com/google/go-licenses/issues/73#issuecomment-1005587408
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worthwhile to document examples of the types of URLs that are problematic (I didn't get a clear sense what "major branch conventions" meant from the comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let me expand on the comment and leave a TODO in v2 roadmap to document known caveats.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in #73 (comment), note I have already built workarounds for these special cases in follow up PRs.

licenses/library_test.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
@Bobgy Bobgy changed the title feat(v2): csv command using module info feat(v2): commands using module info Jan 23, 2022
@Bobgy Bobgy changed the title feat(v2): commands using module info feat(v2): use go modules info Jan 23, 2022
@Bobgy Bobgy force-pushed the v2-csv-1 branch 4 times, most recently from b265450 to c4d4297 Compare January 23, 2022 11:53
csv.go Outdated Show resolved Hide resolved
licenses/git.go Show resolved Hide resolved
main.go Show resolved Hide resolved
licenses/library.go Outdated Show resolved Hide resolved
csv.go Outdated
Comment on lines 72 to 79
// Using ", " to join words makes vscode/terminal recognize the
// correct license URL. Otherwise, if there's no space after
// comma, vscode interprets the URL as concatenated with the
// license name after it.
// Also, the extra spaces does not affect csv syntax much, we
// can still copy the csv text and paste into Excel / Google
// Sheets.
if _, err := fmt.Fprintln(os.Stdout, strings.Join([]string{lib.Name(), licenseURL, licenseName}, ", ")); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to push back on this. The CSV spec says that spaces should be preserved, so this is potentially a breaking change for readers of the file that aren't expecting the space (example).

Happy to explore other options to support editor/terminal friendly outputs in another PR though!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I'll discuss it separately

@Bobgy
Copy link
Collaborator Author

Bobgy commented Feb 24, 2022

TODO: revert git.go, because we will still use it in the future.

@Bobgy Bobgy force-pushed the v2-csv-1 branch 3 times, most recently from b79dc6f to 7a12b72 Compare February 24, 2022 22:45
@Bobgy Bobgy requested a review from wlynch February 24, 2022 22:47
Copy link
Contributor

@wlynch wlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last comment, otherwise lgtm!

csv.go Outdated
}
}
// Remove the "*/vendor/" prefix from the library name for conciseness.
if err := writer.Write([]string{unvendor(lib.Name()), licenseURL, licenseName}); err != nil {
if _, err := fmt.Fprintln(os.Stdout, strings.Join([]string{lib.Name(), licenseURL, licenseName}, ",")); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i'd just revert to the previous csv.Writer behavior.

There's a few other quirks in CSV behavior that while I'm not 100% sure we need to worry about, it's easier to just trust the csv library to handle it properly instead of rolling our own.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, just reverted this.

@Bobgy Bobgy requested a review from wlynch February 26, 2022 01:18
@Bobgy Bobgy changed the title feat(v2): use go modules info feat!(v2): use go modules info Feb 28, 2022
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.

2 participants