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

Add REST Importer for Jetbrains / VSCode #435

Merged
merged 1 commit into from
Jan 1, 2025

Conversation

benfaerber
Copy link
Contributor

@benfaerber benfaerber commented Dec 30, 2024

Description

A few months ago I filed a pull request to add this in but I got busy so it ended up getting closed. (Addresses this) Anyway, I'm back and decided to reintegrate it! Here is the previous closed issue with more details: #205

I made the REST Parser a separate crate because it was quite complex and would be useful for other projects as well. You can check it out here: https://github.com/benfaerber/rest_parser

The old version had a Jetbrains environment loading feature which I did not include because it added a ton of complexity and not too much benefit.

Import using:

slumber import rest ./test_data/rest_http_bin.http ./test_data/rest_slumber.yml
slumber import vscode ./test_data/rest_http_bin.http ./test_data/rest_slumber.yml
slumber import jetbrains ./test_data/rest_http_bin.http ./test_data/rest_slumber.yml

Known Risks

  • Adding a new crate
  • Touching the importer

QA

Lots of unit tests in rest.rs and some test data.

Checklist

  • [ X] Have you read CONTRIBUTING.md already?
  • [ X] Did you update CHANGELOG.md?
    • Only user-facing changes belong in the changelog. Internal changes such as refactors should only be included if they'll impact users, e.g. via performance improvement.
  • [ X] Did you remove all TODOs?
    • If there are unresolved issues, please open a follow-on issue and link to it in a comment so future work can be tracked

Copy link
Owner

@LucasPickering LucasPickering left a comment

Choose a reason for hiding this comment

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

Nice work! This is looking great, just some formatting/readability comments mostly. I'm going to push one commit with some small changes I made to the docs, I figured it would be easier than trying to explain.

There are some formatting and lint issues as well, they'll show up in the CI once I kick it off.

Thanks for your work on this!

CHANGELOG.md Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
docs/src/cli/import.md Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Show resolved Hide resolved
@benfaerber
Copy link
Contributor Author

Cool, I think these changes address everything mentioned. Now I just have to figure out the linters.

Copy link
Owner

@LucasPickering LucasPickering left a comment

Choose a reason for hiding this comment

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

Few more suggestions but it's looking good! For the formatting you can run cargo fmt locally, for linting cargo clippy will show you the errors.

crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
crates/import/src/rest.rs Outdated Show resolved Hide resolved
@benfaerber
Copy link
Contributor Author

There we go! That should be everything. I got the linter and formatter happy too.

Copy link
Owner

@LucasPickering LucasPickering left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you! This will go out in 2.5.0, which will probably be next week some time.

@LucasPickering LucasPickering merged commit c7be11b into LucasPickering:master Jan 1, 2025
15 checks passed
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.

2 participants