-
Notifications
You must be signed in to change notification settings - Fork 7
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!: improve gherkin files #161
Conversation
9fbb85d
to
72acea4
Compare
relates to #166 |
8c3de77
to
6f5055d
Compare
Signed-off-by: Simon Schrottner <[email protected]>
49ba22e
to
dc3a944
Compare
Signed-off-by: Simon Schrottner <[email protected]>
dc3a944
to
796c0e8
Compare
6804205
to
e91f61f
Compare
open-feature/java-sdk-contrib#1115 and open-feature/python-sdk-contrib#121 and open-feature/js-sdk-contrib#1129 Those need to have a little bit more work still, as they're not 100% feature complete, but show the process in general |
e91f61f
to
2e6d291
Compare
@toddbaert @beeme1mr I would love to move forward with this. I have implemented reference implementations within Java and Python (also using tags to exclude features), which work great (except for Python 3.8, which is a little sad, but I can't fix the tooling). (teaser: I also have a socket path implementation for Python with this test suite ready) Let me know what you think :) |
1bd95d3
to
169e533
Compare
Signed-off-by: Simon Schrottner <[email protected]>
169e533
to
53bfd58
Compare
gherkin/connection.feature
Outdated
Then the error event handler should have been executed | ||
Then the ready event handler should have been executed | ||
|
||
Examples: Stable |
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.
This is a bit of a nit, but if I understand correctly, we can use Scenarios
here instead of Examples
which I think is just clearer (using SSL or a target URI seems more like a specific "scenario" to me than an "example". Example just sounds like example data, but this is a different mode of operation, not just different sample data.
If this is a difficulty, I'm fine with Examples
, but my understanding is they are synonymous in cucumber.
gherkin/targeting.feature
Outdated
@@ -0,0 +1,146 @@ | |||
@rpc @in-process @targeting | |||
Feature: flagd json evaluation |
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 we should update this name to match the file name better, or vice-versa... maybe "flag custom targeting features"
gherkin/evaluation.feature
Outdated
@@ -0,0 +1,34 @@ | |||
@rpc @in-process | |||
Feature: flagd providers |
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.
Same here, can we make the filename and Feature a bit more similar?
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 looked at the draft implementations and they look good, so I don't have any major compatibility concerns. I left some notes about minor discrepancies I found between some content and file names, and also suggest some alternate keywords for clarity.
Flag evaluation now differs from the spec gherkin file. We could also adapt the spec gherkin file or create one here; I am open to both solutions.
I think we should update the spec gherkin, this format looks better.
Breaking Change
I think another thing we should do is make this a 1.0 release... mostly because we mostly (only?) use this internally and then we could more easily express breaking changes going forward. You can do this by adding "release-as": "1.0.0"
in the release please config here:
"release-type": "simple", |
Signed-off-by: Simon Schrottner <[email protected]>
I looked at our Gherkin files, and I feel we can optimize them slightly. They're sometimes hard to use.
When
intoGiven