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

[Experiment] get something working with new Value base class. #266

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

botandrose
Copy link
Contributor

@botandrose botandrose commented Oct 5, 2023

Description

Refactor Test::Case to use a new base class of Value, which encapsulates the idea of an immutable value object instantiated by keywords. This is very similar to the new Data class included in Ruby 3.2: https://docs.ruby-lang.org/en/3.2/Data.html, but with the extra feature of optional parameters.

This has a number of benefits: it resolves a number of Rubocop violations, it replaces long positional arguments with keyword arguments, it removes boilerplate, and makes explicit that these objects are Value Objects. https://en.wikipedia.org/wiki/Value_object

This is just a proof-of-concept, but if this looks like a direction we want to go in, I'll continue this PR with the following:

  1. Document Value
  2. Write specs for Value
  3. Convert the other value objects in core, as well.
  4. Remove Rubocop exceptions that are no longer relevant after this.
  5. Follow-up PRs for gems depending on core to use the new kwargs syntax where necessary.

Thoughts?

Type of change

  • Refactoring (improvements to code design or tooling that don't change behaviour)
  • Breaking change (will cause existing functionality to not
    work as expected)

I guess its both? Refactoring from the outer cucumber standpoint, but breaking API change from the core gem standpoint.

  • Tests have been added for any changes to behaviour of the code
  • New and existing tests are passing locally and on CI
  • bundle exec rubocop reports no offenses
  • RDoc comments have been updated
  • CHANGELOG.md has been updated

@luke-hill
Copy link
Contributor

Some of this is quite non-standard e.t.c. - Is it worth maybe just holding off some of this stuff if it's gonna get this complex until ruby 3.2 is the minimum, there's 1000 other tech debt things to fix meanwhile.

@botandrose
Copy link
Contributor Author

@luke-hill Seeing as this data structure is included in Ruby 3.2 -- and in core, not std lib -- I'd say its as standard Ruby as it gets, as standard as Hash or Set.

We're currently supporting Ruby 2.5+, released in 2017, nearly six years ago. This is an attempt to get the benefits described above without waiting six-ish years for cucumber to require 3.2+.

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Reviewed 4/5 files. Just not the specific Value class. No issues from my POV.

I have managed to tackle some of the rubocop complexities so there is likely 1 or 2 conflicts here. Also I'm not sure what kind of message you want to put in the Changelog (if any).

@botandrose
Copy link
Contributor Author

@luke-hill Great! Thank you very much. I'll rebase and continue with the TODO items I listed in the description.

@luke-hill
Copy link
Contributor

Heya @botandrose

I've been working (And will be slowly), on a minor release to cucumber-ruby to use all the new work we've done in core and other associated gems.

Naturally spec failures are abundant, but I've been fixing things up a bit. I also did a patch fix to core (not yet released).

Work is here: cucumber/cucumber-ruby#1751 - Not sure if it will impact anything. Don't think so.

@botandrose
Copy link
Contributor Author

Hey @luke-hill thanks for the heads-up!

I'm really looking forward to that release. That line number matching regression has kept me stuck on cucumber v3, and so I'll finally be able to rejoin the rest of the world on a modern cucumber release!

Once I'm dogfooding modern cucumber again, I'll be more motivated to hack on it.

Regarding that, and this PR specifically, I've made some explorations expanding this data structure to the other value object classes. The jury is still out on whether or not I want to actually recommend this PR for merging, but I hope to get back to it again soon.

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.

3 participants