remove js.Wrapper and add time.Time convenience function#17
remove js.Wrapper and add time.Time convenience function#17nitram509 wants to merge 10 commits intonorunners:mainfrom
Conversation
see Go 1.18 release notes: https://tip.golang.org/doc/go1.18
go.mod
Outdated
|
|
||
| go 1.16 | ||
|
|
||
| require github.com/corbym/gocrest v1.0.5 // indirect |
There was a problem hiding this comment.
It's not possible to remove this one - it would break the provided test below.
I guess we're talking about introducing a new dependency here?
I'm sorry I forgot to mention it in the main description.
May I propose adding this Gocrest library, as it significantly improves the readability of tests.
Here's an example
then.AssertThat(testing, "hi", is.EqualTo("bye").Reason("we are going"))that's way more expressive, compare to the vanilla Golang style of writing if ... then t.fail()...
What do you think?
Would it be ok to leave the test as is?
There was a problem hiding this comment.
This library is simple enough to have zero dependencies. I'll look into updating your tests if you like.
There was a problem hiding this comment.
Hi, thank you for answering.
Well, from engineer to engineer, I can't agree on "This library is simple enough to have zero dependencies."
Since this is a "test only" dependency and test-support libraries like this have the tendency to be stable over a long period of time. The major benefit I see is improved expressiveness when reading tests.
Hence, I consider the value of following the clean code principle is the stronger goal,
in relation to zero dependencies (as in all dependencies).
(just my 2cents)
That said, this is your lib, and you have the last word about the goals and the non-goals of this lib.
If you wish, I can rework towards "vanilla go" and remove the dependency as well, or you do this ... as you wish.
Happy to chat further, if you want :)
|
Thank you for the contribution. |
…arnings: "Node.js 12 actions are deprecated. For more information see: https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/. Please update the following actions to use Node.js 16: actions/setup-go@v2, actions/checkout@v2" remove former empty line to trigger Github Action
|
Hi @norunners Additionally, I did fix the Github workflow, because of the warnings (see the screenshot attached). May I ask, how you think about the test library 'gocrest' used? (see my previous comment)
|
Co-authored-by: Caleb Jasik <calebjasik@jasik.xyz>
|
would it be possible to split this PR into multiple? one fixing go 1.18 support && one adding the testing library w/ nice helpers? |
|
@jasikpark @norunners I did my best in splitting this PR into three separate ones #18 #19 and #20. |
3 participants
Hi,
this is a great lib. While tinkering around, I found two issues, I would like to share/improve.
This PR covers two aspects.
Any feedback is welcome.