-
Notifications
You must be signed in to change notification settings - Fork 252
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
added support for Attachments (aka Embedddings) #623
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
==========================================
- Coverage 83.21% 79.69% -3.52%
==========================================
Files 28 40 +12
Lines 3413 3113 -300
==========================================
- Hits 2840 2481 -359
- Misses 458 513 +55
- Partials 115 119 +4 ☔ View full report in Codecov by Sentry. |
… formats - done by sneaking the attachment into the context.Context object (but not letting that context value propagate into the next step, so it's kinda bending the context.Context purpose)
@Johnlon thanks for this, it looks great! Are you able to take a look at why the build is failing? It seems to be just some noise from |
@mattwynne thanks - will do. |
At go fmt sorry . |
Hey @Johnlon no worries, looks like the build on main is green which is the main thing I was worried about, so all good. As I said in the other thread, we favour Ship/Show/Ask in the Cucumber org, so feel free to merge your own PRs if you feel you've had enough feedback. Try and do it on purpose though! 😆 |
I really appreciate the work you've put into this, thanks! |
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.
Thank you! Sorry, I'm late to the party, but probably we can consider/apply a few changes on top of this PR (with a new PR) before we tag a new version (to avoid backwards compatibility concerns).
No prob happy to help |
First things first, before thinking about a release, if you can make another PR with the changes @vearutop has suggested, then we'll have the code in the best shape we can get it. |
… (#628) * support multiple calls to the Attach() function from a single step * run_progress_test.go changed so it's not sensitive to the name of the clone target directory * applied code review comments also added _example/attachments * applied code review comments also added _example/attachments * applied code review comments also added _example/attachments
🤔 What's changed?
The cucumber and events formatters support the inclusion of Attachments.
The proposed API is demonstrated in the tests as ...
⚡️ What's your motivation?
#617
Want support for Attachments from steps.
This PR proposes to use the context.Context object as a means to attach content from the step.
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
This change involves minimal changes to patterns of use of godog API's.
I added tests for the cuke and events formats but the coverage isn't being recorded even on some lines I didn't modify.
fmt_output_test.go now has a normalise() fn that eliminates some extraneous diffs in the actual/expected files such as line endings and also line numbers so that the tests are easier to maintain and cross platform,
📋 Checklist: