Finalize 'active' configuration subset split #234
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
haxscramper
force-pushed
the
compiler-cli-parser-separation
branch
2 times, most recently
from
42a6bf5
to
3beb92b
Compare
* Changes - Move missing 'input' configuration parameters to the ~CurrentConf~ - Added documentation for most of the configuration variables like ~git.url~, ~doc.item~, ~<module>.always~, module search logic - Add tests for the CLI parser. - Add ~[FileIndex]~ implementation for the ~ConfigRef~ to avoid constantly writing ~conf.m.fileInfos[index.int32]~ as if we don't have templates/procedures to take care of repetitive code. - Closes nim-works#210 - it is hardly possible to gain anything by refactoring this part of the compiler. I updated documentation on some of the relevant pieces I found, as well as some minor tests, but otherwise there is nothing to gain here. Current implementation is usable for the purposes of nim-works#211 - it is possible to call into ~parseCommand()~ and get self-contained chunk of data for all the flags, that we can also do roundtrips on (using nim-works#213). * Tried to do but failed Due to large number of dependencies between data storage, reporting etc. it was not possible to really separate CLI parser logic from everything else. I tried separating them out into standalone files, but it was a lot more troubles than it worth: - Moving CLI parser would've require separating 'external' reports into different file, but this would still pull in ~ast_types.nim~, which is a pretty large dependency on its own. - CLI parser can do external reports - but it is possible to have multiple failures before processing terminates if there is a ~--errormax=N~ before. So logical idea of - "CLI flag is either valid or invalid" does not really cut here, since "invalid" can be postponed. - Because there is still a need to drag around whole suite of configuration state, reporting "deprecated" warnings can stay as it is now.
haxscramper
force-pushed
the
compiler-cli-parser-separation
branch
from
3beb92b
to
7084ad6
Compare
saem
reviewed
Feb 21, 2022
|
|
||
| let csd = currentSourcePath().parentDir() | ||
|
|
||
| suite "Path options specification": |
There was a problem hiding this comment.
Anecdote/Tangent:
This sort of testing is an important step forward, pretty early when I started working on nimsuggest I was told about findNimProjectFile or some proc like that. Anyhow, the intention was detecting a project file so the vscode extension can infer things in a better way than having to specify a config.
Long story short, findNimProjectFile is busted and the whole thing is buggy as well. It really needs a spec and this test case shows a pretty good way of how I can now enumerate that.
So at the time I didn't have a great way of testing it and no one was a fan of adding these sort of tests to the compiler so I ended up dropping it. This PR reminded me of that, so now at least there is a way forward. 🎉
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Due to how this thing is implemented, it is hardly possible to separate the mutation part from everything else. So I ended up just writing more documentation and some tests for these parts.