Skip to content

move math constants to standard library#9678

Merged
sholderbach merged 9 commits intonushell:mainfrom
amtoine:move-math-constants-to-standard-library
Sep 5, 2023
Merged

move math constants to standard library#9678
sholderbach merged 9 commits intonushell:mainfrom
amtoine:move-math-constants-to-standard-library

Conversation

@amtoine
Copy link
Copy Markdown
Member

@amtoine amtoine commented Jul 13, 2023

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 math rather than math

what can be done

> use std; $std.math
> use std math; $math
> use std *; $math

will all give

╭───────┬────────────────────╮
│ GAMMA │ 0.5772156649015329 │
│ E     │ 2.718281828459045  │
│ PI    │ 3.141592653589793  │
│ TAU   │ 6.283185307179586  │
│ PHI   │ 1.618033988749895  │
╰───────┴────────────────────╯

and the following will work too

> use std math E; $E
2.718281828459045
> use std math *; $GAMMA
0.5772156649015329

what can NOT be done

looks like every export works fine now 😌

Tests + Formatting

After Submitting

@amtoine amtoine added the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Jul 13, 2023
@amtoine amtoine force-pushed the move-math-constants-to-standard-library branch from aac3ef1 to 52fd600 Compare July 14, 2023 07:51
# return the Euler-Mascheroni constant
#
# see https://en.wikipedia.org/wiki/Euler%27s_constant
export def gamma [] { 0.5772156649015329 }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

feels like these should be export const but not sure if we support that yet

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

very good catch, i think we do now 😮

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done in 57da755

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i've updated the description of the PR with the possible usage 👍

Note
use std math; $math does not work, but i think it should 🤔

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Aug 7, 2023

marking as DRAFT until the CI passes... ♻️

@amtoine amtoine marked this pull request as draft August 7, 2023 11:44
amtoine added 3 commits August 7, 2023 13:49
…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`
@amtoine amtoine marked this pull request as ready for review August 7, 2023 12:31
Example {
description: "Apply the cosine to π",
example: "math pi | math cos",
example: "3.141592 | math cos | math round --precision 4",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

agree 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@sholderbach
is there a way to allow these example tests to use std math PI?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 👀

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i've seen other examples using math round, so i went for the safest route 😌

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Aug 18, 2023

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.

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Aug 20, 2023

let's wait for #10049 to land then 😌

@amtoine amtoine marked this pull request as draft August 20, 2023 09:55
amtoine pushed a commit that referenced this pull request Aug 20, 2023
<!--
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.
-->
@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Aug 23, 2023

@amtoine Should be unblocked now

…to-standard-library

this should bring the latest const / module changes and unblock the PR.
@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Aug 26, 2023

@kubouch
i've updated the PR, tested again and updated the PR description 👍
there are good things but still some other unexpected commands that do not work apparently 🤔

fixed in e662f31

@amtoine

This comment was marked as outdated.

this fixes the export issues apparently
@sholderbach
Copy link
Copy Markdown
Member

What is the status on this one since consts can be exported from modules?

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Sep 5, 2023

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 😋

@amtoine amtoine marked this pull request as ready for review September 5, 2023 16:56
@sholderbach
Copy link
Copy Markdown
Member

Let's give it a go

@sholderbach sholderbach merged commit 456e2a8 into nushell:main Sep 5, 2023
@amtoine amtoine deleted the move-math-constants-to-standard-library branch September 5, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants