SimonBaeumer / goss

Quick and Easy server testing/validation
Apache License 2.0
26 stars 2 forks source link

Replace cli.Context with app.CliContext #55

Closed SimonBaeumer closed 5 years ago

SimonBaeumer commented 5 years ago

fixes #54 fixes #53

SimonBaeumer commented 5 years ago

@au-phiware thanks for your review! :+1:

This is a worthwhile exercise, in my view. I think using a struct makes things clearer, but I don't think it should be in a internal package. Maybe you agree, I don't know, maybe you have bigger plans or just want to see where this road goes, that's ok too. This struct could probably be split up into a number of structs according to the package that needs it.

I totally agree with you and like the idea about separating the structs. This will go in line with reducing some of it's features/usages. At some points there are some funny things like a no-color and color option which do exactly the same - just inversed, see validate.go:54.

An alternative approach could be the WithOption functional style that's common in the standard libraries.

Could you give me some resources on this? I never heard of it until now and could only find some blog articles in Scala where I wasn't quite sure if you meant that.

au-phiware commented 5 years ago

Certainly, this is a nice explaination: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

That article sites this one as the source of the idea: https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html?m=1

SimonBaeumer commented 5 years ago

Very good article, I like the idea and will try to implement it.

SimonBaeumer commented 5 years ago

@au-phiware Yeah, continued development and tried the WithOptions. The WithOptions way is very nice but for this case it added too much overhead to create all these functions (and variadic functions are a little bit annoying in a process where a method`s input params change every minute^^) For now I added some more structs as you suggested, and it worked very very well:

It now makes it already easier to debug. There are still a lot of flaws which need to be removed/changed. The next step is to move the properties to their correct responsibilities, i.e. Package should be moved from GossRunTime to the package resource. This will make the structs and method signatures a lot easier.

Maybe you find time for a little bit feedback! :+1:

au-phiware commented 5 years ago

This sounds great. You are right, the WithOption is better suited to options the change infrequently or only in edge case scenarios. I'll try to take a look at the code tomorrow, but sounds like you've made some good progress.

codeclimate[bot] commented 5 years ago

Code Climate has analyzed commit da201429 and detected 10 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 7
Duplication 3

The test coverage on the diff in this pull request is 37.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 23.1% (9.2% change).

View more on Code Climate.