Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Rewrites simple modules to TypeScript #1545

Merged
merged 9 commits into from
Oct 22, 2019
Merged

Rewrites simple modules to TypeScript #1545

merged 9 commits into from
Oct 22, 2019

Conversation

artem-zakharchenko
Copy link
Contributor

@artem-zakharchenko artem-zakharchenko commented Oct 8, 2019 •
edited
Loading

🚀 Why this change?

  • To slowly begin to rewrite Dredd internal modules to TypeScript
  • Rewrites and simplifies sortTransactions module

📝 Related issues and Pull Requests

✅ What didn't I forget?

  • To write docs
  • To write tests
  • To put Conventional Changelog prefixes in front of all my commits and run npm run lint
  • Add unit tests for sortTransactions module

@artem-zakharchenko artem-zakharchenko force-pushed the ts-port-01 branch 2 times, most recently from 9e87b4c to 3c50cf6 Compare October 9, 2019 08:52
honzajavorek
honzajavorek suggested changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

I don't like the RESTMethod name, otherwise seems okay.

packages/dredd/lib/general.ts Outdated Show resolved Hide resolved
packages/dredd/lib/general.ts Outdated Show resolved Hide resolved
packages/dredd/test/unit/sortTransactions-test.ts
import { expect } from 'chai';
import sortTransactions from '../../lib/sortTransactions';
import { Transaction, RESTMethod } from '../../lib/general';

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a similar test written in JS being removed. Does it mean you want to keep it to be sure nothing got broken, or that it doesn't exist and this part isn't tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find any existing test that would cover sortTransactions. To my knowledge this adds the missing unit test, so there is no JavaScript predecessor to remove. Please, point me if you know where this behavior may be tested already. Thanks.

@honzajavorek
Copy link
Contributor

If you add two methods, you can close this one #267

@artem-zakharchenko
Copy link
Contributor Author

@honzajavorek thanks for the review.

Are you speaking about adding LINK and UNLINK to HTTPMethod? Are those even valid methods per spec?

@honzajavorek
Copy link
Contributor

They are used with proxies.

@artem-zakharchenko
Copy link
Contributor Author

@honzajavorek I think we can cover LINK/UNLINK separately. Thanks!

@artem-zakharchenko artem-zakharchenko merged commit ddcc77e into master Oct 22, 2019
@artem-zakharchenko artem-zakharchenko deleted the ts-port-01 branch October 22, 2019 13:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@honzajavorek honzajavorek honzajavorek approved these changes

Assignees
No one assigned
Labels
in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@artem-zakharchenko @honzajavorek