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

Mosaic#39 add --debug flag #41

Merged
merged 3 commits into from
Nov 13, 2020
Merged

Mosaic#39 add --debug flag #41

merged 3 commits into from
Nov 13, 2020

Conversation

denismaxim0v
Copy link
Member

No description provided.

Copy link
Member

@TheLostLambda TheLostLambda left a comment

Choose a reason for hiding this comment

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

This is looking pretty good to me! I left one nitpick and it might be nice to have some tests, but I'll be honest, tests for this seem a bit challenging to write.

Regardless, thanks a ton for writing this up! I'll probably defer to @imsnif to make the final call though :)

src/pty_bus.rs Outdated
pid.to_string(),
String::from(".log"),
]
.join(""),
Copy link
Member

Choose a reason for hiding this comment

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

Just a very tiny Rust nitpick, but for joining with an empty string, there is the .concat() method that might be a bit more idiomatic :)

@denismaxim0v
Copy link
Member Author

Great, let me know what you end up with

@imsnif
Copy link
Member

imsnif commented Nov 13, 2020

@denismaxim0v - this looks great to me. It will also be extremely helpful in debugging compatibility issues.
I played with this around a little on my machine and it definitely does the right thing. One can use these logs to debug issues by simply doing cat <logfile> inside a mosaic pane (and outside for comparison). The mosaic-logs folder is re-created on startup. All looks good.

@denismaxim0v - I think if you just apply @TheLostLambda's suggestion, we can merge this.

Thanks for this!

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.

3 participants