Skip to content

Conversation

@gmpinder
Copy link
Member

@gmpinder gmpinder commented Jan 18, 2025

So from testing out errors, I've found that putting bool checks into a variable makes it so that we can take the span of the bool variable so that when we run into an error, we can supply that span to the error. This makes it easier for users to see how the check is happening.

Example of one of the error messages:

image

@gmpinder gmpinder self-assigned this Jan 18, 2025
@gmpinder gmpinder force-pushed the nushell/files branch 14 times, most recently from eae8233 to 26467d3 Compare January 19, 2025 00:59
@xynydev
Copy link
Member

xynydev commented Jan 19, 2025

Nice! Are we planning to write all modules in Nushell going forward? Should probably discuss.

If and since the files@v1 and files@v2 introduced in this PR use the exact same syntax and support the same featureset, what is the reason for keeping the bash version around?

@gmpinder
Copy link
Member Author

Nice! Are we planning to write all modules in Nushell going forward? Should probably discuss.

If and since the files@v1 and files@v2 introduced in this PR use the exact same syntax and support the same featureset, what is the reason for keeping the bash version around?

I figured I'd version it mostly cause I wanted to drop the old schema in favor of only the source and destination schema.

@gmpinder gmpinder marked this pull request as ready for review January 19, 2025 16:40
@xynydev
Copy link
Member

xynydev commented Jan 19, 2025

I figured I'd version it mostly cause I wanted to drop the old schema in favor of only the source and destination schema.

Oh yeah, I didn't notice that. Sounds good. That means we have to announce this change, so we better bundle it with #336. Also, versioned modules need separate README's for each version. Feel free to look at what's happening with the default-flatpaks PR.

@xynydev
Copy link
Member

xynydev commented Jul 26, 2025

I wonder if there's a way to avoid breaking everyone's builds unexplainably when this is merged... Maybe do the same thing as default-flatpaks and allow the old schema to be used with this version of the module, then just do a fancy error in code to catch it?

Or maybe we should just implement support for the old schema completely, but print a deprecation notice when using it. Would that hurt?

@fiftydinar
Copy link
Collaborator

fiftydinar commented Jul 26, 2025

There are no special advantages to Nushell files module compared to bash version, right?
I see only better error message.
On the other hand, default-flatpaks Nushell module is made to be vastly more reliable than the bash one, as it's a run-time module.

Build-time modules like this are pretty simple & bash is not the show-stopper.
So in my opinion, this PR can stay open until some more compeling reason for merging is made.

For example, maybe it would be useful to add removing files/folders feature in this new module:
#247

Or maybe we should just implement support for the old schema completely, but print a deprecation notice when using it. Would that hurt?

That should be done imo, like we do with gnome-extensions. files is a crucial module used by everyone.

@xynydev
Copy link
Member

xynydev commented Jul 26, 2025

In my opinion, the better error messages are a great feature, though I doubt many people are having issues with this module.

So, maybe we can actually keep this PR open, not spend v2 on such a minor change, and brainstorm new features to include. Then before merge, we can implement a fancy error message for people attempting to use the old configuration.

Quick ideas:

type: files@v2
copy:
  - source: system
    destination: /
remove:
  - /etc/profile.d/ublue-os-just.sh
create:
  - destination: /etc/profile.d/gtk-theme.sh
    content: GTK_THEME=adw-gtk3

Possible other stuff: setting file permission bits, ?

@gmpinder
Copy link
Member Author

These new features sound like they would be useful

@fiftydinar
Copy link
Collaborator

fiftydinar commented Nov 21, 2025

Since bootc support for multiple distributions is cooking nicely outside of Fedora Atomic, we need to make our modules more compatible for all of them.
So we need to either rewrite them in POSIX using shell built-ins, or to use nushell.

Given that this is in Nushell, I think that we can update this a bit if needed, merge this and implement new features later if desired.

And of course, notify users about it.

I'll make an issue on this.

Edit:
#503

@gmpinder
Copy link
Member Author

I nearly forgot about this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants