Skip to content

Fix setting configuration params#629

Merged
sophiajt merged 2 commits intonushell:masterfrom
est31:fix_setting_config
Sep 9, 2019
Merged

Fix setting configuration params#629
sophiajt merged 2 commits intonushell:masterfrom
est31:fix_setting_config

Conversation

@est31
Copy link
Contributor

@est31 est31 commented Sep 9, 2019

Fixes #627

Fixes a regression caused by #579, specifically commit cc8872b .

The code was intended to perform a comparison between the wanted
output type and "Tagged" in order to be able to provide a
special-cased path for Tagged. When I wrote the code, I
used "name" as a variable name and only later realized that it
shadowed the "name" param to the function, so I renamed it to
type_name, but forgot to change the comparison.
This broke the special-casing, as the name param only contains
the name of the struct without generics (like "Tagged"), while
std::any::type_name (in the current implementation) contains
the full paths of the struct including all generic params
(like "nu::object::meta::Taggednu::object::base::Value").

Fixes nushell#627

Fixes a regression caused by nushell#579, specifically commit cc8872b .

The code was intended to perform a comparison between the wanted
output type and "Tagged<Value>" in order to be able to provide a
special-cased path for Tagged<Value>. When I wrote the code, I
used "name" as a variable name and only later realized that it
shadowed the "name" param to the function, so I renamed it to
type_name, but forgot to change the comparison.
This broke the special-casing, as the name param only contains
the name of the struct without generics (like "Tagged"), while
`std::any::type_name` (in the current implementation) contains
the full paths of the struct including all generic params
(like "nu::object::meta::Tagged<nu::object::base::Value>").
@est31
Copy link
Contributor Author

est31 commented Sep 9, 2019

I would like to add a regression test here but have no idea how to add one. Running config --set would change files on the file system, which is not a good idea to do during tests.

@sophiajt
Copy link
Contributor

sophiajt commented Sep 9, 2019

@est31 - I was wondering that too. At some point, we may have a way of mocking changes to the configuration that we can test against.

For now, though, I think it's okay to just fix it and test for what we can.

@sophiajt sophiajt merged commit 8e3b7e2 into nushell:master Sep 9, 2019
Hofer-Julian pushed a commit to Hofer-Julian/nushell that referenced this pull request Jan 27, 2023
* Update installation.md

Clarifies what `--features=extra` actually includes

* tweaked wording

Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
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.

Regression in setting vi mode

3 participants