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

cratification: move the bytes command to nu-cmd-extra #9509

Merged
merged 6 commits into from Jun 23, 2023
Merged

cratification: move the bytes command to nu-cmd-extra #9509

merged 6 commits into from Jun 23, 2023

Conversation

stormasm
Copy link
Contributor

now that #9455 has landed we can move the bytes command to nu-cmd-extra

in concert with

moving nu_command::util to nu-cmd-base

@stormasm stormasm requested a review from amtoine June 23, 2023 00:03
Comment on lines 1 to 12
pub(crate) mod add;
pub(crate) mod at;
pub(crate) mod build_;
pub(crate) mod bytes_;
pub(crate) mod collect;
pub(crate) mod ends_with;
pub(crate) mod index_of;
pub(crate) mod length;
pub(crate) mod remove;
pub(crate) mod replace;
pub(crate) mod reverse;
pub(crate) mod starts_with;
Copy link
Member

Choose a reason for hiding this comment

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

What is the need to make this blanket visible? There have been recently a number of compiler warnings due to potential namespace conflicts due to many similar named symbols imported via use module::* that arise from symbols being public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sholderbach thanks great catch ! I made your requested changes....

@stormasm stormasm removed the request for review from amtoine June 23, 2023 15:11
@stormasm
Copy link
Contributor Author

@amtoine I know you are moving this weekend and @sholderbach kindly reviewed the PR so I am going to go ahead and land this...

I am going to circle back on to the bits commands after this lands and make the same changes @sholderbach suggested for this PR on the visibility issue of not needing the blanket pub(crate) which I corrected in this PR thanks to his suggestion.

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Should be good but marked as a breaking change as it removes commands from the default build

@sholderbach sholderbach added the pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes label Jun 23, 2023
@stormasm stormasm merged commit d1449c4 into nushell:main Jun 23, 2023
17 of 20 checks passed
@stormasm stormasm deleted the bytest2 branch June 23, 2023 19:23
stormasm added a commit that referenced this pull request Jun 24, 2023
In the spirit of #9509 we are changing the visibility of the bits
commands
to align with the way they are correctly done in the bytes commands...

Thanks to @sholderbach for pointing out this nice improvement
to me in my PR earlier in the day...

```rust
mod and;
mod bits_;
mod not;
mod or;
mod rotate_left;
mod rotate_right;
mod shift_left;
mod shift_right;
mod xor;
```
@amtoine
Copy link
Member

amtoine commented Jun 25, 2023

thanks @stormasm and @sholderbach for backing me up on that one, really appreciate 🐘 ❤️

@amtoine
Copy link
Member

amtoine commented Jun 25, 2023

and it looks all good and working as expected to me, nice job @stormasm 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:breaking-change 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.

None yet

3 participants