Skip to content

make the themes a module#648

Merged
amtoine merged 4 commits intonushell:mainfrom
amtoine:make-themes-a-module
Oct 23, 2023
Merged

make the themes a module#648
amtoine merged 4 commits intonushell:mainfrom
amtoine:make-themes-a-module

Conversation

@amtoine
Copy link
Copy Markdown
Member

@amtoine amtoine commented Oct 20, 2023

related to

description

with nushell/nupm#33, Nupm can now install separate modules from a single package 🥳

the nu_scripts are a package and looks like they contain a few potential standalone modules:

  • the themes
  • the hooks
  • the custom completions
  • ...

this PR is a proposal to move themes/ to nu-themes/ and add ./nu-themes/ as a module to the nu-scripts package.
that way when running nupm install --path . in the root of the nu_scripts, a nu-themes module will be installed in $env.NUPM_HOME/modules.

then one can run

use nu-themes/themes/nushell-dark.nu

and next get the theme with

nushell-dark

@amtoine amtoine requested a review from kubouch October 20, 2023 13:47
amtoine added a commit to amtoine/dotfiles that referenced this pull request Oct 20, 2023
@amtoine
use nushell-dark theme back
76b1b1e 
related to
- nushell/nu_scripts#648

will require to install `nu-scripts` with Nupm from nushell/nu_scripts#648.
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 20, 2023

seems reasonable

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Oct 20, 2023

seems reasonable

note that there will be more renaming like this if we go that route 😏

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Oct 20, 2023

I think you can just rename themes to nu-themes and keep the files as-is? Then, you'd have just use nu-themes/spam-theme.nu. Also, I think it's missing mod.nu for Nushell to recognize nu-themes as a module. nupm is currently very dumb and will blindly copy what you tell it to, but I imagine in the future it might have some validation.

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Oct 21, 2023

I think you can just rename themes to nu-themes and keep the files as-is?

yup, that's the biggest part of this PR, it's just an mv themes/ nu-themes/ 😉

Then, you'd have just use nu-themes/spam-theme.nu. Also, I think it's missing mod.nu for Nushell to recognize nu-themes as a module. nupm is currently very dumb and will blindly copy what you tell it to, but I imagine in the future it might have some validation.

i wouldn't do that for now 👀
the issue with adding a mod.nu to nu-themes/ is that, when you use one of the theme, aaaaaall the module will be parsed which takes way to much time to be put in a config...
right now, i think the most direct and fastest method is to not add a mod.nu and run

use nu-themes/themes/nushell-dark.nu

🤔

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Oct 21, 2023

I see, yeah, it makes sense to access the files directly. mod.nu wouldn't prevent that, you'd still be able to access them individually, but that's ok, we can keep it without.

I was just wondering why not have simply nu-themes/nushell-dark.nu, why is there the themes directory in between.

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Oct 21, 2023

I see, yeah, it makes sense to access the files directly. mod.nu wouldn't prevent that, you'd still be able to access them individually, but that's ok, we can keep it without.

👍

I was just wondering why not have simply nu-themes/nushell-dark.nu, why is there the themes directory in between.

because there are not only themes in nu-themes, also a script, a README and screenshots 🤔
this is purely to make nu-themes/ easy to look at 😋

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Oct 21, 2023

may we land this as an experiment? 😇

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Oct 21, 2023

Ah, OK, sorry, I'm dumb :-D . I looked wrong and somehow thought you added an extra themes/ subdirectory. One last comment is that now you'd install the screenshots/ as well, not sure if it's desired. But if you're OK with it, feel free to land it.

@amtoine amtoine mentioned this pull request Oct 22, 2023
Merged
@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Oct 22, 2023

Ah, OK, sorry, I'm dumb :-D . I looked wrong and somehow thought you added an extra themes/ subdirectory. One last comment is that now you'd install the screenshots/ as well, not sure if it's desired. But if you're OK with it, feel free to land it.

that's a very good point and i don't like it 🤔

i've tried a fix in a Nupm PR just above.
changes here would be

  • move the directories back and rename the inner theme directory to nu-themes
mv nu-themes/ themes
mv themes/themes/ themes/nu-themes
  • change $.modules in package.nuon to [./themes/nu-themes]

the new Nupm would install the ./themes/nu-themes/ directory in $env.NUPM_HOME/modules/nu-themes/, what do you think? 😇

@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Oct 22, 2023

additional nice thing is that one would have to run

use nu-themes/nushell-dark.nu

instead of

use nu-themes/themes/nushell-dark.nu

now 😋

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Oct 22, 2023

Yeah, changing the $.modules to themes/nu-themes should work fine.

@amtoine
move nu-themes to themes and inner themes to nu-themes
1c18a95 
commands used
```nushell
mv nu-themes/ themes
mv themes/themes/ themes/nu-themes
```
@amtoine
fix the path to the module in package.nuon
27e81f4
@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Oct 23, 2023

Yeah, changing the $.modules to themes/nu-themes should work fine.

done in 27e81f4 😉

let's land this and see how it goes 🥳

@amtoine amtoine merged commit cc0719b into nushell:main Oct 23, 2023
@amtoine amtoine deleted the make-themes-a-module branch October 23, 2023 16:25
@amtoine
Copy link
Copy Markdown
Member Author

amtoine commented Oct 23, 2023

i've tried to nupm install it and it works like a charm 😌 💪

@amtoine amtoine mentioned this pull request Oct 23, 2023
Merged
amtoine added a commit to amtoine/dotfiles that referenced this pull request Oct 23, 2023
@amtoine
use the new structure of nu-themes in nu-scripts
91f038d 
related to:
- nushell/nu_scripts#648 (comment)
@amtoine amtoine mentioned this pull request Oct 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kubouch kubouch Awaiting requested review from kubouch

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@amtoine @fdncred @kubouch