move math constants to standard library#9678
Conversation
aac3ef1 to
52fd600
Compare
crates/nu-std/std/math.nu
Outdated
| # return the Euler-Mascheroni constant | ||
| # | ||
| # see https://en.wikipedia.org/wiki/Euler%27s_constant | ||
| export def gamma [] { 0.5772156649015329 } |
There was a problem hiding this comment.
feels like these should be export const but not sure if we support that yet
There was a problem hiding this comment.
very good catch, i think we do now 😮
There was a problem hiding this comment.
i've updated the description of the PR with the possible usage 👍
Note
use std math; $mathdoes not work, but i think it should 🤔
|
marking as DRAFT until the CI passes... ♻️ |
…to-standard-library-ci this commit allows to define constants in modules and test this PR in the REPL. related to nushell#9773
this commit allows the use of the constants from outside `std math`
| Example { | ||
| description: "Apply the cosine to π", | ||
| example: "math pi | math cos", | ||
| example: "3.141592 | math cos | math round --precision 4", |
There was a problem hiding this comment.
Grr, I see why it is necessary but I dislike the impact on the educational value of those examples, when they get stuffed with evermore unrelated parts to let them work as tests.
There was a problem hiding this comment.
@sholderbach
is there a way to allow these example tests to use std math PI?
There was a problem hiding this comment.
also we'd need the use of math round because the precision of Nushell floats is not enough for commands like math sin to evaluate PI to integers 👀
There was a problem hiding this comment.
Not a strict blocker, so we could go ahead in shrinking the command bloat.
The test example machninery uses some precision threshold for floats if I recall but I would need to check.
There was a problem hiding this comment.
i've seen other examples using math round, so i went for the safest route 😌
There was a problem hiding this comment.
The example could say use std math pi; $pi | .... The example test might not run, though, so the test might go to a separate test or into std test suite.
|
I think this PR is blocked on the fact that constant exporting does not work recursively. #9773 only exports const from a top-level module but does not go to submodules. |
|
let's wait for #10049 to land then 😌 |
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> #9773 introduced constants to modules and allowed to export them, but only within one level. This PR: * allows recursive exporting of constants from all submodules * fixes submodule imports in a list import pattern * makes sure exported constants are actual constants Should unblock #9678 ### Example: ```nushell module spam { export module eggs { export module bacon { export const viking = 'eats' } } } use spam print $spam.eggs.bacon.viking # prints 'eats' use spam [eggs] print $eggs.bacon.viking # prints 'eats' use spam eggs bacon viking print $viking # prints 'eats' ``` ### Limitation 1: Considering the above `spam` module, attempting to get `eggs bacon` from `spam` module doesn't work directly: ```nushell use spam [ eggs bacon ] # attempts to load `eggs`, then `bacon` use spam [ "eggs bacon" ] # obviously wrong name for a constant, but doesn't work also for commands ``` Workaround (for example): ```nushell use spam eggs use eggs [ bacon ] print $bacon.viking # prints 'eats' ``` I'm thinking I'll just leave it in, as you can easily work around this. It is also a limitation of the import pattern in general, not just constants. ### Limitation 2: `overlay use` successfully imports the constants, but `overlay hide` does not hide them, even though it seems to hide normal variables successfully. This needs more investigation. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> Allows recursive constant exports from submodules. # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect -A clippy::result_large_err` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
|
@amtoine Should be unblocked now |
…to-standard-library this should bring the latest const / module changes and unblock the PR.
This comment was marked as outdated.
This comment was marked as outdated.
this fixes the export issues apparently
|
What is the status on this one since consts can be exported from modules? |
might have forgotten to mark that as ready for review, my bad 🙏 if we're ok with the examples in the PR description and the open thread above, then let's go 😋 |
|
Let's give it a go |
Description
we talked about this before in some meetings so i thought, why not?
the hope is that these constants do not require Rust code to be implemented and that this move will make the Rust source base a bit smaller 🤞
User-Facing Changes
mathematical constants (e, pi, tau, phi and gamma) are now in
std mathrather thanmathwhat can be done
will all give
and the following will work too
what can NOT be done
looks like every export works fine now 😌
Tests + Formatting
After Submitting