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.

Sign up for GitHub

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

Jump to bottom

Enhance the help command to print info about a given unit #267

Merged
merged 13 commits into from
Nov 24, 2023
Merged

Enhance the help command to print info about a given unit #267

merged 13 commits into from
Nov 24, 2023

Conversation

eminence
Copy link
Contributor

This PR starts to implement the enhanced help suggestion in #238.

At the moment, it looks like this:

>>> help megabyte
  Byte
  https://en.wikipedia.org/wiki/Byte
  Aliases: B, byte, bytes, Byte, Bytes, octet, octets, Octet, Octets
  A unit of [DigitalInformation]
  
  1 megabyte = 1_000_000 byte
  1 byte = 8 bit

>>> help m
  Metre
  https://en.wikipedia.org/wiki/Metre
  Aliases: metre, metres, meter, meters, m
  A unit of [Length]
  
  metre is a base unit

CC #238

eminence
eminence commented Nov 22, 2023
View reviewed changes
numbat/src/lib.rs Show resolved Hide resolved
numbat/src/unit.rs Show resolved Hide resolved
@eminence eminence marked this pull request as draft November 22, 2023 23:26
@eminence
Copy link
Contributor Author

The second commit adds support for attaching metadata to local variables and into the help command:

>>> help planck_constant
  Variable: planck_constant
  Planck constant
  https://en.wikipedia.org/wiki/Planck_constant
  Aliases: planck_constant, ℎ
  
      = 6.62607e-34 J/Hz

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat-cli/src/main.rs Outdated Show resolved Hide resolved
sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Really nice — thank you very much!

I added some additional metadata for math constants.

@eminence eminence force-pushed the enhanced_help branch 3 times, most recently from 81b412b to fcbd5ce Compare November 23, 2023 15:03
@eminence eminence marked this pull request as ready for review November 23, 2023 15:28
@eminence eminence changed the title Draft: Enhance the help command to print info about a given unit Enhance the help command to print info about a given unit Nov 23, 2023
@eminence
Copy link
Contributor Author

Ok, given that I didn't see any objections to the approach taken here, I think this is ready for a fuller review. I want to take another pass at the output formatting of the help output (I welcome any and all comments on this)

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat-cli/src/main.rs Outdated Show resolved Hide resolved
sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/bytecode_interpreter.rs
Comment on lines +388 to +396
pub fn get_defining_unit(&self, unit_name: &str) -> Option<&Unit> {
self.unit_name_to_constant_index
.get(unit_name)
.and_then(|idx| self.vm.constants.get(*idx as usize))
.and_then(|constant| match constant {
Constant::Unit(u) => Some(u),
_ => None,
})
}
Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/decorator.rs
Comment on lines +70 to +71
pub fn contains_aliases_with_prefixes(decorates: &[Decorator]) -> bool {
for decorator in decorates {
Copy link
Owner

Choose a reason for hiding this comment

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

minor:

Suggested change
pub fn contains_aliases_with_prefixes(decorates: &[Decorator]) -> bool {
for decorator in decorates {
pub fn contains_aliases_with_prefixes(decorators: &[Decorator]) -> bool {
for decorator in decorators {

or is "decorates" the correct term here? :-)

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/lib.rs Outdated Show resolved Hide resolved
sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/lib.rs
return help;
}

m::text("Not found")
Copy link
Owner

@sharkdp sharkdp Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It would be fantastic if we could also use this for types, i.e. do info ElectricCharge and get information about how it is defined and what the base representation is. But I guess we can leave this for another PR. I think it's also far less important than units and constants.

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/lib.rs Outdated
let mut help =
m::text("Unit: ") + m::unit(md.name.as_deref().unwrap_or(keyword)) + m::nl();
if let Some(url) = &md.url {
help += m::string(url) + m::nl();
Copy link
Owner

Choose a reason for hiding this comment

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

I think we could maybe put the URL after the name? And similar for variables.

>>> info C
  Unit: Coulomb (https://en.wikipedia.org/wiki/Coulomb)
  Aliases: coulomb, coulombs, C
  A unit of [ElectricCharge]
  
  1 coulomb = 1 A x s

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/lib.rs Outdated
if !prefix.is_none() {
help += m::nl()
+ m::value("1 ")
+ m::type_identifier(keyword)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be

Suggested change
+ m::type_identifier(keyword)
+ m::unit(keyword)

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/lib.rs Outdated
+ m::text(" = ")
+ m::value(prefix.factor().pretty_print())
+ m::space()
+ m::type_identifier(&full_name);
Copy link
Owner

Choose a reason for hiding this comment

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

Same here:

Suggested change
+ m::type_identifier(&full_name);
+ m::unit(&full_name);

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/lib.rs Outdated
if let Some(BaseUnitAndFactor(prod, num)) = x {
help += m::nl()
+ m::value("1 ")
+ m::type_identifier(&full_name)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
+ m::type_identifier(&full_name)
+ m::unit(&full_name)

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/lib.rs Outdated
+ m::text(" = ")
+ m::value(num.pretty_print())
+ m::space()
+ prod.pretty_print_with(|f| f.exponent, 'x', '/', true);
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this always uses type_identifier for the markup? In that case, maybe we can pass the markup style as an argument (see FormatType)? Or pass m::type_identifier/m:unit as a function argument?

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
numbat/src/lib.rs Outdated
+ prod.pretty_print_with(|f| f.exponent, 'x', '/', true);
} else {
help +=
m::nl() + m::type_identifier(&full_name) + m::text(" is a base unit");
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
m::nl() + m::type_identifier(&full_name) + m::text(" is a base unit");
m::nl() + m::unit(&full_name) + m::text(" is a base unit");

sharkdp
sharkdp reviewed Nov 23, 2023
View reviewed changes
Copy link
Owner

@sharkdp sharkdp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Really cool. We should also think about the web version. I can also make the necessary changes if you can't run it, for example.

Maybe we can also add a simple integration test in numbat-cli/tests/integration.rs (see similar test for help: help_text).

@eminence
Copy link
Contributor Author

Ok I think I addressed all the review comments. Here's a screenshot showing how it looks now:

image

(please also make sure 341ca6d is OK)

@sharkdp
Copy link
Owner

sharkdp commented Nov 23, 2023

(please also make sure 341ca6d is OK)

👍

@eminence
Copy link
Contributor Author

New commits: a cleanup on how variables are shown, an integration test, and support in the wasm version

@sharkdp sharkdp merged commit c2f89b6 into sharkdp:master Nov 24, 2023
14 checks passed
@krehel krehel mentioned this pull request Dec 19, 2023
Merged
@eminence eminence mentioned this pull request May 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharkdp sharkdp sharkdp left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eminence @sharkdp