Skip to content
This repository has been archived by the owner on Aug 2, 2024. It is now read-only.

Proof highlighting #204

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Proof highlighting #204

wants to merge 7 commits into from

Conversation

sudoPom
Copy link

@sudoPom sudoPom commented Apr 12, 2022

Proofs produced by the modus proof command will now contain literals that are coloured the same way that the user's terminal colours directories, so it is guaranteed to be easily visible to the user. This PR should close #185 .

Before fix:

Screenshot_20220412_165013

After fix:

Screenshot_20220412_165040

@mechtaev
Copy link
Collaborator

Thanks, @sudoPom! @thevirtuoso1973, can you please review it?

@mechtaev
Copy link
Collaborator

As I remember, star dependencies are not allowed if we want to publish our library on crates.io:

https://github.com/modus-continens/modus/blob/5199e3acd8409a1ba3a1b02b53787ce9e5bf5f03/modus-lib/Cargo.toml#L24

@sudoPom
Copy link
Author

sudoPom commented Apr 12, 2022

Will do!

* Ansi term version
* Itertools re added (whoops)
Comment on lines 536 to 545
fn get_color(color_of: &str) -> ansi_term::Style{
let colors = LsColors::from_env().unwrap_or_default();
let mut style: std::option::Option<&lscolors::Style>;
match color_of{
"Directory" => style = colors.style_for_indicator(lscolors::Indicator::Directory),
"Normal" => style = colors.style_for_indicator(lscolors::Indicator::Normal),
_ => panic!("The only used LSColours are 'Directory' and 'Normal'")
};
style.map(Style::to_ansi_term_style).unwrap_or_default()
}
Copy link
Contributor

@thevirtuoso1973 thevirtuoso1973 Apr 12, 2022

Choose a reason for hiding this comment

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

This would be a nice place to define a mapping from nodes to colors.
But for now, I suggest changing the match to something like

"ImagePred" => blah blah directory...
_ => blah blah normal...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we probably don't need to unwrap/panic over a missing color env, so pick some default if possible.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, I'll add this soon :)

@thevirtuoso1973
Copy link
Contributor

I like the idea of using directory color.

Also generally use rustfmt and check any notable clippy suggestions.

Comment on lines -713 to -739
/// Returns a normalized version of a resolution error --- should be better for hash equality.
/// Without this, may get a lot of resolution errors, for example, that are identical except for a different
/// auxiliary variable index.
fn normalize(self) -> ResolutionError {
match self {
ResolutionError::UnknownPredicate(l) => {
ResolutionError::UnknownPredicate(l.normalized_terms())
}
ResolutionError::InsufficientGroundness(ls) => ResolutionError::InsufficientGroundness(
ls.into_iter().map(|x| x.normalized_terms()).collect(),
),
ResolutionError::MaximumDepthExceeded(ls, s) => ResolutionError::MaximumDepthExceeded(
ls.into_iter().map(|x| x.normalized_terms()).collect(),
s,
),
ResolutionError::BuiltinFailure(l, s) => {
ResolutionError::BuiltinFailure(l.normalized_terms(), s)
}
ResolutionError::InsufficientRules(l) => {
ResolutionError::InsufficientRules(l.normalized_terms())
}
ResolutionError::InconsistentGroundnessSignature(sigs) => {
ResolutionError::InconsistentGroundnessSignature(sigs.to_vec())
}
ResolutionError::NegationProof(l) => {
ResolutionError::NegationProof(l.normalized_terms())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

also, you're probably aware but this (and another change) shouldn't be removed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better Proof Highlighting
3 participants