-
Notifications
You must be signed in to change notification settings - Fork 2
Upgrade to use updated dependency-submission-toolkit #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want a +1 from someone better versed in Typescript but this all LGTM, nice change!
@@ -0,0 +1,104 @@ | |||
import { parseGoList, parseGoModGraph } from './parse' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to lie, I kind of hate putting test files right alongside normal files ;) But I can let it fly...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's takes all all types! I think I've more strongly adopted this working on a TypeScript+Go project, where unit tests in Go must be collocated with the source files. So apologies for ry
Since this is to be open sourced, I'm inclined to keep the tests collocated because this is the common idiom for Jest test organization (it's the example given in the Getting Started guide, for instance). But I fully bless internal TS projects using other organization mehtods.
I hope we can at least agree that that __test__/
directory (another Jest practice) is an abomination ;)
.github/workflows/go-action.yml
Outdated
@@ -16,4 +15,4 @@ jobs: | |||
uses: dsp-testing/go-snapshot-action@lsep/go-list-impl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I knew it was too good to be true that it worked on the first run 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! But it did work! 🎉 https://github.com/dsp-testing/go-snapshot-action/runs/6836365970?check_suite_focus=true
Updating to point to main
, which it will run once merged
Updates to the latest dependency-submission-toolkit
Additionally:
I anticipate rework, but my goal is to get this installed in GitHub repos soon so we can start exercising our staff-ship more. If things can be addressed in future iterations, let's save it for later.