Skip to content

Conversation

Kissaki
Copy link
Contributor

@Kissaki Kissaki commented Nov 1, 2024

Both errored when executed.

@Kissaki Kissaki force-pushed the fix/mod-create-examples branch from 7bee9ca to 96beae9 Compare November 1, 2024 18:41
@fdncred fdncred merged commit 6ba5776 into nushell:main Nov 1, 2024
2 checks passed
@fdncred
Copy link
Contributor

fdncred commented Nov 1, 2024

Thanks

@NotTheDr01ds
Copy link
Contributor

NotTheDr01ds commented Nov 2, 2024

@Kissaki Thanks! I'm glad you were able to work through the chapter and find these issues. Apologies for the first one - Definitely my copy/paste error there.

On the second one, it was actually working "as intended" (but not quite well-enough, apparently). What I think must have happened is that you (at some point) imported it with:

use my-utils/mod.nu *

Maybe an extra Tab-complete too far picked up the mod.nu?

If I'm correct, then this would have caused $env.CURRENT_FILE to return the mod.nu rather than the directory name, leading to the error you saw.

Importing with use my-utils * as in the doc should (at least in my testing, and re-testing, and re-testing yet some more ;-) properly return the directory name and cd to it successfully.

With your change, it now fails when properly imported - It doesn't error, but it returns the wrong directory (the parent of the module directory itself).

But you've found an interesting issue, but at least one that I can work around with a change to check if the CURRENT_FILE ends in mod.nu or not.

If you get a chance, can you (a) confirm my suspicions about use my-utils/mod.nu *, and (b) that my fix in #1610 works? Thanks!

@Kissaki
Copy link
Contributor Author

Kissaki commented Nov 2, 2024

@NotTheDr01ds

I retested the code without the fixup in this MR, with various constellations, and I always get the error. use my-utils, use my-utils *, use my-utils\mod.nu, use my-utils\mod.nu *, examine-module, my-utils examine-module. I made sure to use new sessions without previous use module context.

Do you expect $env.CURRENT_FILE to not be a file path? Or that cdworks with a file path?

For me it makes sense that it never works because CURRENT_FILE is a file path, and cd change directory requires a directory path. Navigating into a file makes no sense.

I changed it to cd ($env.CURRENT_FILE | path dirname) with that understanding. Getting the directory path of the file. If it's mod.nu then it's the module's directory. If it's a module file, not mod.nu, then it would list those (likely unintended behavior). The example is with a module folder though, so makes sense there.

With that context, I don't see how #1610 makes sense. If it's not mod.nu, then it's not a module directory with files in the first place. cd-ing into a file path and listing its content makes no sense to me.

@NotTheDr01ds
Copy link
Contributor

NotTheDr01ds commented Nov 2, 2024

While I'm still not sure why we are getting different results, I think the core problem goes back to this Nushell issue, but I still want to make sure.

I thought the fix for that issue had made it in 0.99, but it appears it was reverted, so it's likely I'll need to find an entirely different example for that stage.

I changed it to cd ($env.CURRENT_FILE | path dirname) with that understanding.

And given what you were/are seeing, that makes perfect sense!

Do you expect $env.CURRENT_FILE to not be a file path? Or that cdworks with a file path?

In my testing, when a module is imported using directory-form, the CURRENT_FILE is the name of the directory. I see that it is the file when use my-utils/mod.nu * is used, but I haven't (yet) found other cases where it is. I've also tested on both Linux and Windows with the same results, but I'm still looking.

It's possible there's more than one root issue here.

@Kissaki Kissaki deleted the fix/mod-create-examples branch April 12, 2025 16:20
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.

3 participants