Added GetGherkinFeatures method to godog.Suite #276
Conversation
Codecov Report
@@ Coverage Diff @@
## main #276 +/- ##
==========================================
+ Coverage 82.30% 84.19% +1.89%
==========================================
Files 26 16 -10
Lines 2368 2468 +100
==========================================
+ Hits 1949 2078 +129
+ Misses 309 271 -38
- Partials 110 119 +9
Continue to review full report at Codecov.
|
suite.go
Outdated
| @@ -248,6 +264,16 @@ func (s *Suite) Step(expr interface{}, stepFunc interface{}) { | |||
| s.steps = append(s.steps, def) | |||
| } | |||
|
|
|||
| // GetGherkinFeatures returns parsed Gherkin features for the suite | |||
| // It can be useful to retrieve some information about Gehrkin document, Pickles ... | |||
| func (s *Suite) GetGherkinFeatures() []GherkinFeature { | |||
There was a problem hiding this comment.
What do you think about instead of this function returning interfaces, it would return structs which includes the GherkinDocument and the Pickles?
I'm not too happy about returning pointers, sense that would mean that the test could modify the data, I would like to keep potential side effects to a minimum.
There was a problem hiding this comment.
Sometimes the developer writing the tests may want to modify some data before the test. This interface way would be a good way to give this possiblity. As it stays in the suite, it would be done before the whole testing.
Or would it make to much side effects ?
But I can change the GherkinFeature to a struct instead right with 2 attributes for pickles and gherkindocument
There was a problem hiding this comment.
Shouldn't it be better to make the interface but add some explicit way to modify data so we can assume the developer now what he is doing if something break ?
There was a problem hiding this comment.
I would say it is a bit too much, and would add more maintenance ...
but a comment on the interface warning about modifications could do or ?
@lonnblad what do you think ?
|
@radtriste thanks for the PR, it would be good to get some tests for this as well |
|
That I will write :) |
|
@lonnblad is that what you meant ? (see my changes) |
|
any news on that one ? |
|
@radtriste so sorry for the delay getting this merged. Would you be able to help rebase it, then we can get it merged in? |
|
@mattwynne sure will try to rebase in the next days ! |
radtriste
force-pushed
the
issue_222
branch
2 times, most recently
from
c483a0f
to
50a949b
Compare
|
@mattwynne @lonnblad let me know what you think of the change. |
|
Hi @radtriste, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
|
Thanks @radtriste. I've merged your PR without a very thorough review as we're short on regular maintainers at the moment. Now that you have the commit bit 🎉 it would be awesome if you had any time to help us with that problem. No pressure! ❤️ |
|
@mattwynne Cannot promise anything but can keep an eye on godog project for now |
And created interface GherkinFeature to allow to retrieve the main Gherkin information.
#222
Following #223 and migration to new Gherkin structure.