Add @include directive to nimconf#1550
Conversation
The recent code changes primarily involve refactoring the NimConfParser to utilize a shared 'identCache' variable instead of passing it explicitly through multiple function arguments. This adjustment is a preparatory step for implementing an `include` directive in the configuration parsing, a feature that aims to enhance modularity and flexibility in configuration management.
Introduce an `include` directive to the Nim configuration parser, aimed at enhancing the configurability and modularity of Nim's configuration files. The rationale behind this feature is to facilitate the inclusion of additional configuration files, organizing configurations better and promoting reuse across different projects. ## Changes 1. **New `include` Directive**: The addition of handling for the `include` directive within the `parseDirective` procedure allows for the parsing and inclusion of external configuration files. Users can now specify other config files to be loaded, enhancing the flexibility of configuration management. 2. **Circular Inclusion Prevention**: The `readConfigFile` function was updated to check if a configuration file is already included before loading it again. This prevents circular inclusion scenarios that could lead to infinite recursion or errors. 3. **Lexer State Management**: To ensure that the lexer state remains intact while reading an included config file, the code saves the current lexer state and restores it afterward. This safeguard ensures that the main parsing process is not disrupted by the inclusion of additional files.
The code changes made in `nimconf.nim` focus on optimizing the handling of directives within the `NimConfParser`. The primary modification involves the decision to parse directives selectively, rather than attempting to parse them for every token encountered. This change addresses inefficiencies in the way directives were previously processed, which could lead to unnecessary computations when tokens were not directives. ## API Changes None. ## Impl Changes 1. The removal of the `confTok` procedure, which was used to handle token reading and directive parsing. 2. In its place, a template `nextTok` was introduced to streamline token retrieval, cutting down on redundant logic. 3. The parser now checks if a token starts with "@" and only calls `parseDirective` when appropriate, thereby reducing overhead, improving performance, and fixing a sigsegv. ## Fixes 1. A nimconf statement of form `--ident@[ident]` fails with a SIGSEGV.
Reasons: - easier to maintain - discoverability - fight entropy
|
Thank you for the PR, @quantimnot. An include feature for configuration files seems useful, especially for projects structured as a monorepo. What's the status on this? Is it ready for review? Or is there work still remaining?
There were many problems with NimScript-based configuration, including slowness and general implementation issues, and since it also didn't fit in with the direction w.r.t. to the build process, it was removed. It'll will most likely come back one day, though in a somewhat different form, where the script is driving the compiler directly, instead of just configuring some settings. |
|
Status? It was a lot of code ago when I did it. I think it is complete enough. I recall growing aggravated with finding more original design/impl bugs and having to stop using my time on it. It works, so I wanted to see if it would be useful to others. However, over that last couple of days, the thought occurred to me that maybe the best thing for Nimskull is to waste no time on old cruft, but implement views, cps -> implement new general parsing/... modules -> new config system. If you think it is worth your precious time, then I think it is an improvement on the current state. Also, for some reason of which I can't recall, I added Nimscript support to your Testament in the branch. I can PR that if you think it's of value. |
|
That's a lot of red. I think it's failing because I left the nimscript test rewrites in there, but I removed the testament changes. I will address this only if say you want this :) |
|
Thank you for the info, and sorry for the delayed response. While I - personally - don't have an immediate use for includes in configuration files, I think others might, and from a quick skim, the changes also seem small and local enough to warrant the feature. To be clear, I'd would merge the I know that this is not a very strong expression of "want", and if it's not enough to motivate you, that's totally fine and I'd very much understand.
I don't think it's needed. NimScript has very little remaining uses, having been made obsolete by the dedicated VM backend for the most part, and thus doesn't warrant |
|
To add a data point, I know a few folks, myself included, that would like to have an |
3 participants
Summary
In Nim, I use a modular nimscript setup for my projects. I really wish nimscript was still part of Nimskull. Not having it was a hurdle and turnoff to me, and probably others.
A while back, I improved the nimconf impl to at least give me some modularity. Here it is. Also, I found and fixed some crash[es], but I don't recall the details.