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 "Inspect" section to display debug values #132

Merged
merged 12 commits into from
Mar 28, 2024

Conversation

afternoon2
Copy link
Contributor

@afternoon2 afternoon2 commented Feb 10, 2024

  • add ctx.inspect()

Copy link
Owner

@abey79 abey79 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 a great feature!

I have yet to give it a test, but I already have a bit of a gripe with name configurability: I'd much rather do without. I do agree that "Debug" may sound a bit too low-level/"scary" for something that might show up in release/final sketches. Instead, we could name the panel "Inspect" and the function ctx.inspect().

crates/whiskers/src/runner/mod.rs Outdated Show resolved Hide resolved
crates/whiskers/src/runner/debug.rs Outdated Show resolved Hide resolved
crates/whiskers/src/runner/debug.rs Outdated Show resolved Hide resolved
@@ -17,6 +18,7 @@ use std::sync::Arc;
use crate::Sketch;
pub use animation::AnimationOptions;
use convert_case::Casing;
pub use debug::DebugOptions;
Copy link
Owner

Choose a reason for hiding this comment

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

Not pub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here it's pub because it's used by the context. but I removed it from lib.rs and prelude.rs. is it ok?

@afternoon2
Copy link
Contributor Author

As a user, I'd still opt for a rename possibility (inspect sounds less alarming, but still in a "checking if everything's fine" manner rather than presenting program's valid features). And if not via the runner's method, then maybe with ctx.inspect_label(label: &str)?

@abey79
Copy link
Owner

abey79 commented Feb 12, 2024

As a user, I'd still opt for a rename possibility (inspect sounds less alarming, but still in a "checking if everything's fine" manner rather than presenting program's valid features). And if not via the runner's method, then maybe with ctx.inspect_label(label: &str)?

This really feels like API clutter, regardless of where it lives. What's the use case here? What problem would that solve?

@afternoon2 afternoon2 changed the title Debug parameters section Inspect variables section Feb 13, 2024
@afternoon2
Copy link
Contributor Author

just a customization option, but I won't fight for it

@afternoon2 afternoon2 marked this pull request as ready for review February 13, 2024 16:02
@afternoon2
Copy link
Contributor Author

ah, forgot to add a label for the PR, sorry 🙏🏻

@abey79 abey79 added whiskers Relates to whiskers or whiskers-derive feature New feature labels Feb 13, 2024
@abey79
Copy link
Owner

abey79 commented Feb 13, 2024

ah, forgot to add a label for the PR, sorry 🙏🏻

No prob, I'm not even sure they are settable by external contributors 🤷🏻

@abey79 abey79 changed the title Inspect variables section Add "Inspect" section to display debug values Mar 28, 2024
@abey79 abey79 merged commit b36f963 into abey79:master Mar 28, 2024
10 checks passed
@abey79
Copy link
Owner

abey79 commented Mar 28, 2024

Sorry for the humongous delay! 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature whiskers Relates to whiskers or whiskers-derive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants