Shell icon indicating copy to clipboard operation
Shell copied to clipboard

[Feature Request] Extend `import` functionality

Open martin-rueegg opened this issue 2 years ago • 7 comments

Currently, the following import capabilities are possible

  • only relative paths are allowed.
  • import of an entire main block with {block} import {file}, e.g. images import 'imports/images.nss'
  • import within the main blocks
    • var
    • static
    • dynamic
    • not sure about set, set/theme, etc.

Please consider implementing the support for (if not already supported):

  1. absolute paths, at least local ones, starting with a drive letter, i.e. C:\some\path\config.nss or D:\config.nss.
    This is required for the use of expressions like @user.appdata.
  2. expressions (variables and functions) e.g. @sys.programdata, @user.appdata or user-defined functions available in the current scope.
  3. globing e.g. to include all files in a directory: C:\some\path\*.nss.
    Or, if globing is too complex, then maybe if the path points to a directory, include all *.nss files in that directory.
    In either case, sort the files alphabetically before processing.
  4. importing shell block i.e. shell import {file} (unless -register {config.nss} is implemented).

martin-rueegg avatar Mar 04 '23 17:03 martin-rueegg

import set/theme

It is already being added

import of an entire main block with

This was also supported 2 days ago

absolute paths

This issue has been fixed

expressions (variables and functions)

Expressions can be used with import

  1. globing and 4. importing shell block

I will try to implement this suggestion

This link has the last build that was developed https://nilesoft.org/download/shell/debug.zip

moudey avatar Mar 04 '23 22:03 moudey

Thank you, @moudey!

import of an entire main block with

This was also supported 2 days ago

I have tested with both v1.8.2 as well as v1.8.2.1 and was not able to import the var block in the following manner:

shell
{
	var import 'metaworx/var.nss'

	// all the other blocks.
}

However, it works like so, with the exact same file (metaworx/var.nss):

shell
{
	var
	{
		import 'metaworx/var.nss'
	}

	// ..
}

absolute paths

This issue has been fixed

Awesome! Works perfectly. 👍

expressions (variables and functions)

Expressions can be used with import

Jep. I don't know why I couldn't get it to work. But works now. 👍

  1. globing and 4. importing shell block

I will try to implement this suggestion

Ok. Thank you.

martin-rueegg avatar Mar 05 '23 22:03 martin-rueegg

Another thing I've noticed: When there is a import in an already imported file, then I need to add an extra closing curly bracket } at the end of the intermediate file. See #187

martin-rueegg avatar Mar 05 '23 22:03 martin-rueegg

Happy to create a separate github issue for this. Shall I?

Yes, because I am currently developing the import, it would be good to stand on this issue

moudey avatar Mar 05 '23 23:03 moudey

Thank you, @martin-rueegg . I am currently working on fixing these issues

moudey avatar Mar 06 '23 18:03 moudey

Hi @martin-rueegg, Please check the import tag issues with this build

shell.nss

import 'imports/extended.nss'

extended.nss

var {
    import 'var.nss'
}

set {
    import 'set.nss'
    static.image=2
    //theme {import 'theme.nss'}
}

images {
    import 'images.nss'
}

static {
    import 'static.nss'
}

dynamic {
    import 'dynamic.nss'
}

or

extended.nss

var import 'var.nss'
set import 'set.nss'
images import 'images.nss'
static import 'static.nss'
dynamic import 'dynamic.nss'

moudey avatar Mar 08 '23 15:03 moudey

Hi @moudey

Thank you so much for this! I've tested several scenarios; here the results of my tests.


shell.nss

The following works:

// works. But not recommended. See below.
import 'imports/extended.nss'

However, I would suggest to not support this syntax, as it is not consistent with the requirement, that the import tag should either

  • be enclosed in an other section (shell in this case), and then not having the section's name in front of the keyword: section { import 'file.nss' }, or
  • it should name the section before the import keyword like so: section import 'file'.

See https://nilesoft.org/docs/syntax#import

In fact, the latte is currently not supported for the root file. Hence the following does not work:

// does not work
shell import 'imports/extended.nss'

This is something I would recommend to change, as explained above.

But then again, this works:

// works
shell {
    import 'imports/extended.nss'
}

Which is great as it follows the suggested syntax! And it allows to have the system-defined config files to be included, and then also the user's extended scripts:

shell {
    // system provide files, which may be overwritten during updates
    import 'imports/extended.nss'

    // my own customization, 
    var import '@user.documents/system/Nilesoft Shell/var.nss'
    dynamic import '@mwxConfigRoot/metaworx/convertMedia.nss'
}

Awesome!


extended.nss

All combinations in extended.nss work perfectly!

Nothing more to say. Other than: Fantastic!


Non-existent import file

Currently, an import instruction that points to a non-existing file, makes the whole thing fail. I would recommend for this case to only write a warning to the log file but then simply ignore the missing file.

IF you would ignore that, you could, for example, ship the following shell.nss with your software ...

shell {
    import 'imports/extended.nss'

    import '@user.profile/shell.nss'
}

As such, the user could simply create a file in %USERPROFILE%\shell.nss and add their customization there.


Multiple inclusion of the same file

It is now no longer possible, to include the same config file multiple times. This is probably to avoid recursion.

However, it is very helpful as you can do sort of modules/snippets, that you want to include in different places. E.g. I have a sub-menu with some dynamic items, that I have included in several dynamic menus. This saves me a lot of writing and maintenance.

In order to allow the inclusion of the same file at different locations, and still prevent circular-reference, you could maybe employ something like an array, where you store the absolute paths of the included files, but also remove them again at the end of every file. And before every import of a new file, check it against the entries in the array.


shell.log

It's great to see the imports here. Is that only for debug builds? - Very helpful indeed.

If in a standard distribution this would not work, then it would be great if this could be enabled, eg. by reloading the configuration with Shift+Ctrl+Right-Click on the Taskbar, rather than just Ctrl+Right-Click.

The log file shows import, just after already imported. Minor issue, but could create confusion.


Conclusion / Summary

Awesome work! Thanks again.

Suggestions:

  • [ ] In shell.nss, remove Support for import 'imports/extended.nss'
  • [ ] In shell.nss, add Support for shell import 'imports/extended.nss'
  • [ ] Ignore non-existing config files with a warning in the log file, rather than aborting the entire parsing
  • [ ] Allow re-import of the same config file, if it does not lead to recursion (see above on how that could be implemented)
  • [ ] Maybe enable import logging with Shift+Ctrl+Right-Click on the Taskbar
  • [ ] (Minor) fix to not report a "successful" import after a import has actually failed (due to non-existing file or recursion)
  • [x] Also fix the issue, where an additional closing bracket would cause explorer to crash (see https://github.com/moudey/Shell/issues/187#issuecomment-1460721355)

Hope this helps. Happy to later update the documentation.

Thanks!

martin-rueegg avatar Mar 08 '23 20:03 martin-rueegg