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

Terminal with code history #123

Merged
merged 10 commits into from Dec 13, 2021
Merged

Conversation

control13
Copy link

Hello,
I found it helpful, when the executed code line or bloc is also printed to the terminal. So I wrote a TerminalWithCode display for that. It works nicely with the fix #122 from @michaelb.
The output then looks like this:

  -------------------OK-------------------
│> print("''lol\"\"")                         
│''lol""

Thank you for this nice project.

I also added a minimum to the documentation. But I have concerns that I messed up the version number the cargo.lock. It shows version 1.0.6. Hmm?

@codecov-commenter
Copy link

Codecov Report

Merging #123 (b413756) into master (ba6c05c) will increase coverage by 0.08%.
The diff coverage is 70.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage   59.34%   59.42%   +0.08%     
==========================================
  Files          35       35              
  Lines        5667     5696      +29     
==========================================
+ Hits         3363     3385      +22     
- Misses       2304     2311       +7     
Impacted Files Coverage Δ
src/display.rs 56.78% <70.00%> (+1.76%) ⬆️
src/lib.rs 58.20% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba6c05c...b413756. Read the comment docs.

@michaelb
Copy link
Owner

Wow, thank you for the nice addition!

There's still a few things that are a bit off (mainly typos & format, but also sniprun shouldn't allow both Terminal and TerminalWithCode to be turned on at the same time), so I'd like to either:

  • redirect your merge onto michaelb:dev so I can merge now & easily fix the minor things later, before this goes into master
  • start a code review, but that may be a bit lengthy and uninteresting for you

Which one do you prefer ?

Also, don't mind about the version numbers in the cargo.toml / cargo lock update status, especially if you're pushing on the dev branch. I update those for each release, and you might just be off-sync

@michaelb
Copy link
Owner

fix #124

@control13
Copy link
Author

control13 commented Dec 12, 2021

Oh nice. Yeah, I prefer the first option (redirect it to dev).

sniprun shouldn't allow both Terminal and TerminalWithCode

Obviously! Totally forgot about that.

Thank you very much for your help.

@michaelb michaelb changed the base branch from master to dev December 13, 2021 17:18
@michaelb michaelb merged commit e03e75a into michaelb:dev Dec 13, 2021
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.

None yet

3 participants