Skip to content

refactor: test_context.go#564

Merged
vearutop merged 3 commits intocucumber:mainfrom
longyue0521:fix_#560
Jun 7, 2023
Merged

refactor: test_context.go#564
vearutop merged 3 commits intocucumber:mainfrom
longyue0521:fix_#560

Conversation

@longyue0521
Copy link
Copy Markdown
Member

@longyue0521 longyue0521 commented May 27, 2023 •
edited
Loading

🤔 What's changed?

  1. ScenarioContext has methods on both value and pointer receivers, make all methods have value receivers.
  2. fix the syntax error in the comment
  3. remove unused function Build

⚡️ What's your motivation?

make the code more cleanable, readable, more compliant with Go language specifications

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)
  • 🐛 Bug fix (non-breaking change which fixes a defect)
  • ⚡ New feature (non-breaking change which adds new behaviour)
  • 💥 Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@longyue0521
Copy link
Copy Markdown
Member Author

longyue0521 commented May 27, 2023 •
edited
Loading

@vearutop please review my changes, thanks.

vearutop
vearutop reviewed Jun 6, 2023
View reviewed changes
test_context.go
// of tested package.
//
// Returns the path to generated executable
func Build(bin string) error {
Copy link
Copy Markdown
Member

@vearutop vearutop Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this function would be a breaking change, let's deprecate it instead.

Copy link
Copy Markdown
Member Author

@longyue0521 longyue0521 Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when add // Deprecated: use xxxx instead we should tell user which function to instead ?

Copy link
Copy Markdown
Member

@vearutop vearutop Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use something free formed like // Deprecated: feature will be removed, no substitute. The message comes from your reasoning why you wanted to remove that function in the first place.

It's ok to keep it as is for now.

@vearutop
Copy link
Copy Markdown
Member

vearutop commented Jun 6, 2023

Hi, thank you, this change seems like a safe simplification. Please restore the Build function and I think it'll be good to merge.

@longyue0521
Copy link
Copy Markdown
Member Author

longyue0521 commented Jun 7, 2023

Hi, thank you, this change seems like a safe simplification. Please restore the Build function and I think it'll be good to merge.

sorry, I don't know which function to instead the Build function, I just rollback it.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 7, 2023 •
edited
Loading

Codecov Report

Merging #564 (9e2dae3) into main (7f75c5d) will not change coverage.
The diff coverage is 64.28%.

@@           Coverage Diff           @@
##             main     #564   +/-   ##
=======================================
  Coverage   82.96%   82.96%           
=======================================
  Files          28       28           
  Lines        3375     3375           
=======================================
  Hits         2800     2800           
  Misses        461      461           
  Partials      114      114
Impacted Files Coverage Δ
internal/formatters/fmt_multi.go 96.72% <ø> (ø)
internal/formatters/undefined_snippets_gen.go 43.39% <ø> (ø)
internal/storage/storage.go 92.10% <0.00%> (ø)
test_context.go 71.73% <63.63%> (ø)
internal/formatters/fmt_base.go 87.28% <100.00%> (ø)

@longyue0521 @vearutop
1. ScenarioContext has methods on both value and pointer receivers, m…
4968017 
…ake all methods have value receivers.

2. fix the syntax error in the comment
3. remove unused function Build

Signed-off-by: longyue0521 <longyueli0521@gmail.com>
@longyue0521 @vearutop
convert ctx (type ScenarioContext) to StepContext
3da92af 
Signed-off-by: longyue0521 <longyueli0521@gmail.com>
@longyue0521 @vearutop
rollback the deleted Build function
9e2dae3 
Signed-off-by: longyue0521 <longyueli0521@gmail.com>
@vearutop vearutop merged commit 72db47c into cucumber:main Jun 7, 2023
@aslakhellesoy
Copy link
Copy Markdown
Contributor

aslakhellesoy commented Jun 7, 2023

Hi @longyue0521,

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:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@longyue0521 longyue0521 deleted the fix_#560 branch June 7, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vearutop vearutop vearutop approved these changes

+1 more reviewer

@aviscode aviscode aviscode approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@longyue0521 @vearutop @aslakhellesoy @aviscode