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

feat: Add output to file option #221

Merged
merged 8 commits into from
Jan 19, 2023
Merged

Conversation

lewandom
Copy link
Contributor

@lewandom lewandom commented Nov 23, 2022

Summary

Fixes #217.

Preflight checklist

  • Code formatted with rustfmt
  • Relevant tests added
  • Any new documentation added - option description

src/utils.rs Outdated Show resolved Hide resolved
Copy link
Owner

@mike-engel mike-engel left a comment

Choose a reason for hiding this comment

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

I'm not sure how easy it is to add tests for filesystem APIs, but have you tried that yet? If not, it would be good to add some tests here

src/translators/decode.rs Outdated Show resolved Hide resolved
@lewandom
Copy link
Contributor Author

lewandom commented Jan 7, 2023

I'm not sure how easy it is to add tests for filesystem APIs, but have you tried that yet? If not, it would be good to add some tests here

I didn't try it yet, but I can certainly take a look. I don't normally code in golang rust, so it can take me a while to research.

@mike-engel
Copy link
Owner

@lewandom thanks, that would be awesome 😄

@lewandom
Copy link
Contributor Author

@mike-engel test added, please review.

@lewandom lewandom requested review from mike-engel and callado4 and removed request for mike-engel and callado4 January 18, 2023 11:04
Cargo.toml Show resolved Hide resolved
@@ -1,14 +1,18 @@
extern crate tempdir;
Copy link
Owner

Choose a reason for hiding this comment

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

is there a reason this is still using the old edition imports?

Suggested change
extern crate tempdir;
use tempdir;

tests/main_test.rs Show resolved Hide resolved
- moved tempdir to dev dependencies
- removed old style import
- refactored exit code logic into main
- covered print_decoded_token in tests
@lewandom
Copy link
Contributor Author

lewandom commented Jan 18, 2023

@mike-engel round 4:

  • it was a bit more involved, since the exit() calls in print_* functions messed with test flow (see Test suite ignores tests after an exit(0). rust-lang/rust#33954). I was in need to factor out exit() usage into the main unit,
  • another debatale change is making TokenOutput public - only for the fact that the test code verifies the decode output by deserializing the output. If that's an issue, let me know if you have another idea around this - last resort I can have the test verify plain Values.

Copy link
Owner

@mike-engel mike-engel left a comment

Choose a reason for hiding this comment

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

Just one minor comment, otherwise this looks good to me. The translators shouldn't have been responsible for exiting anyway, so this is better in the long run

if atty::is(Stream::Stdout) {
println!("{}", jwt);
} else {
print!("{}", jwt);
}
};
exit(0);
Copy link
Owner

Choose a reason for hiding this comment

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

Should this exit be removed now as well?

@lewandom
Copy link
Contributor Author

Done :)

@mike-engel mike-engel merged commit 065fcce into mike-engel:main Jan 19, 2023
@mike-engel
Copy link
Owner

Very sorry for the delay, but this is in 6.0.0 now

@lewandom lewandom deleted the output-file branch June 23, 2023 04:41
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.

Allow saving output to a file instead of stdout
3 participants