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.

Sign up for GitHub

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

Jump to bottom

Feature: hopp-cli in TypeScript #2074

Merged
merged 122 commits into from
Mar 28, 2022
Merged

Feature: hopp-cli in TypeScript #2074

merged 122 commits into from
Mar 28, 2022

Conversation

devblin
Copy link
Contributor

@devblin devblin commented Jan 20, 2022

Hopp-cli

Description

New hopp-cli written in TypeScript

Note: Work in progress

@netlify
Copy link

netlify bot commented Jan 20, 2022 •
edited

👷 Deploy request for hoppscotch pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 1f7a1f1

@lgtm-com
Copy link

lgtm-com bot commented Jan 20, 2022

This pull request introduces 4 alerts when merging d1e7934 into 6b0494d - view on LGTM.com

new alerts:

  • 2 for Unused loop iteration variable
  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jan 25, 2022

This pull request introduces 4 alerts when merging 8c1bfe0 into ee6d173 - view on LGTM.com

new alerts:

  • 2 for Unused loop iteration variable
  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jan 25, 2022

This pull request introduces 4 alerts when merging 5bcbe6a into ee6d173 - view on LGTM.com

new alerts:

  • 2 for Unused loop iteration variable
  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jan 27, 2022

This pull request introduces 6 alerts when merging 2230d90 into 248b6d6 - view on LGTM.com

new alerts:

  • 3 for Unused loop iteration variable
  • 3 for Unused variable, import, function or class

@devblin devblin force-pushed the devblin/hopp-cli branch 2 times, most recently from f4392f6 to e1c8fa5 Compare January 31, 2022 17:01
@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2022

This pull request introduces 6 alerts when merging e1c8fa5 into 98b01b0 - view on LGTM.com

new alerts:

  • 3 for Unused loop iteration variable
  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2022

This pull request introduces 6 alerts when merging b2e14bd into 98b01b0 - view on LGTM.com

new alerts:

  • 3 for Unused loop iteration variable
  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Feb 1, 2022

This pull request introduces 6 alerts when merging 199e2aa into 98b01b0 - view on LGTM.com

new alerts:

  • 3 for Unused loop iteration variable
  • 3 for Unused variable, import, function or class

AndrewBastin
AndrewBastin requested changes Feb 1, 2022
View reviewed changes
Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

Some minor corrections for now. We can simplify the code in a lot of places by relying more on FP-TS stuff.

packages/hoppscotch-cli/src/utils/checks.ts Outdated Show resolved Hide resolved
packages/hoppscotch-cli/src/utils/test-parser.ts Outdated Show resolved Hide resolved
packages/hoppscotch-cli/src/utils/test-parser.ts Outdated Show resolved Hide resolved
@devblin
Copy link
Contributor Author

devblin commented Feb 1, 2022

@AndrewBastin , I have pushed changes with requested changes, and I will do refactoring to simplify using fp-ts.
I am using this fp-ts doc for reference.

AndrewBastin
AndrewBastin previously approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@AndrewBastin
Copy link
Member

The implementation is now good enough for merging, but we are working on setting up the build pipeline to support this. It is gonna take some time for that.

@AndrewBastin AndrewBastin marked this pull request as ready for review March 23, 2022 14:24
@AndrewBastin AndrewBastin self-requested a review March 23, 2022 14:24
AndrewBastin
AndrewBastin approved these changes Mar 28, 2022
View reviewed changes
Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

LGTM 💯

(finally)

@AndrewBastin AndrewBastin merged commit 909d524 into hoppscotch:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewBastin AndrewBastin AndrewBastin approved these changes

Assignees

@AndrewBastin AndrewBastin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@devblin @AndrewBastin @RishabhAgarwal-2001 @liyasthomas @kidonng