Skip to content
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

Embed common packages from monorepo #428

Closed
wants to merge 3 commits into from
Closed

Embed common packages from monorepo #428

wants to merge 3 commits into from

Conversation

vearutop
Copy link
Member

@vearutop vearutop commented Aug 27, 2021

Description

This PR adds a make common target to copy messages and gherkin packages from common monorepo into godog's internal. This is an alternative solution to migrate away from github.com/cucumber/messages-go and github.com/cucumber/gherkin-go while not depending on a monorepo.

Motivation & context

Once common monorepo has an update in gherkin and/or messages, a godog maintainer can run make common and create a PR with produced changes.

Pros:

  • more control for future breaking changes as messages package is not directly importable by end-users
  • no runtime dependency on relatively large common monorepo

Cons:

  • +10k lines of code in godog repo
  • no way for users to import internal/messages in case they need it for advanced step definitions (can be solved by providing more local aliases in godog package)
  • manual upgrade procedure for common packages (though it would still be manual when importing common directly)
  • embedded code does not pass golint and is excluded from linting (may be fixed in monorepo, but with breraking changes in symbol names)
  • overall code coverage drops when merged with embedded packages (can be fixed by improving coverage in monorepo)

Resolves #389.

Type of change

  • Refactoring/debt (improvement to code design or tooling without changing behaviour)
  • Breaking change (will cause existing functionality to not
    work as expected)

Note to other contributors

No notes.

Update required of cucumber.io/docs

No update.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #428 (ba12413) into main (239a424) will decrease coverage by 41.22%.
The diff coverage is 68.91%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #428       +/-   ##
===========================================
- Coverage   81.57%   40.34%   -41.23%     
===========================================
  Files          26       37       +11     
  Lines        2176     6233     +4057     
===========================================
+ Hits         1775     2515      +740     
- Misses        308     3559     +3251     
- Partials       93      159       +66     
Impacted Files Coverage Δ
formatters/fmt.go 100.00% <ø> (ø)
internal/formatters/fmt.go 72.72% <ø> (ø)
internal/formatters/fmt_base.go 86.17% <ø> (ø)
internal/formatters/fmt_cucumber.go 84.76% <ø> (ø)
internal/formatters/fmt_events.go 97.09% <ø> (ø)
internal/formatters/fmt_multi.go 91.66% <ø> (ø)
internal/formatters/fmt_pretty.go 80.56% <ø> (ø)
internal/formatters/fmt_progress.go 93.15% <ø> (ø)
internal/formatters/undefined_snippets_gen.go 41.46% <ø> (ø)
internal/gherkin/dialects_builtin.go 100.00% <ø> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 239a424...ba12413. Read the comment docs.

@mattwynne mattwynne mentioned this pull request Oct 15, 2021
6 tasks
@vearutop
Copy link
Member Author

Closing this, as we need to migrate to different standalone modules (cucumber/common#2029).

@vearutop vearutop closed this Nov 10, 2022
@vearutop vearutop deleted the embed-common branch November 10, 2022 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update cucumber/common module references to use paths instead of subrepos
1 participant