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

fix: add IMethods interface for use in time module functions #2111

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

mannyistyping
Copy link

@mannyistyping mannyistyping commented Aug 14, 2022

🤔 What's changed?

This PR does the following,

  • defines a new interface IMethods in time.ts extending from GlobalTimers
  • explicitly defines the possible return values to beginTiming and endTiming in src/time.ts
    • void for beginTiming
    • number for endTiming
  • updates the methods object to make use of IMethods interface
  • updates timeoutId to use TimerId instead of NodeJS.Timeout
  • a minor fix on link from a previous related PR

⚡️ What's your motivation?

The motivation for this update/change is the incremental completion of #1648 in small manageable PRs.
I imagine this will be one of multiple PRs to replace explicit any with a type.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)

♻️ Anything particular you want feedback on?

  • I would love feedback on this approach to removing any and defining an interface for strictly typing, is this the recommended approach for this project?
    • I based this approach on looking at how other return types were defined and where those definitions were placed
    • Additionally,

📋 Checklist:


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

@mannyistyping mannyistyping force-pushed the issue.1648 branch 2 times, most recently from 58b4892 to 2038608 Compare August 14, 2022 03:58
@coveralls
Copy link

coveralls commented Aug 14, 2022

Coverage Status

Coverage remained the same at 98.236% when pulling 25211ec on mannyistypingonacomputer:issue.1648 into 223e9c4 on cucumber:main.

@mannyistyping mannyistyping changed the title WIP fix: add IMethods interface for use in time module functions Aug 14, 2022
@mannyistyping mannyistyping changed the title fix: add IMethods interface for use in time module functions WIP - fix: add IMethods interface for use in time module functions Aug 14, 2022
@mannyistyping mannyistyping marked this pull request as ready for review August 14, 2022 18:30
@mannyistyping mannyistyping changed the title WIP - fix: add IMethods interface for use in time module functions fix: add IMethods interface for use in time module functions Aug 14, 2022
Copy link
Contributor

@davidjgoss davidjgoss left a comment

Choose a reason for hiding this comment

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

I would love feedback on this approach to removing any and defining an interface for strictly typing, is this the recommended approach for this project?

Yep, this approach is good, and all in keeping with project style. Thanks!

@davidjgoss davidjgoss merged commit 0ac8f58 into cucumber:main Aug 24, 2022
@@ -10,8 +10,6 @@ Please see [CONTRIBUTING.md](https://github.com/cucumber/cucumber/blob/master/CO
## [Unreleased]
### Added
- `IMethods` interface for use in `getTimestamp`, `durationBetweenTimestamps`, and `wrapPromiseWithTimeout` functions and `methods` in `time` module instead of explicit `any` ([#2111](https://github.com/cucumber/cucumber-js/pull/2111))

### Added
Copy link
Author

Choose a reason for hiding this comment

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

🙇‍♂️ Thank you!
I'll be vigilant of this in the future.

@mannyistyping mannyistyping deleted the issue.1648 branch August 24, 2022 19:18
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