-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify string coercion errors #958
Conversation
@@ -205,7 +205,7 @@ instance ( Convertible e t f m | |||
(M.lookup "outPath" s) | |||
_ -> stub | |||
|
|||
fromValue = fromMayToValue $ TString NoContext | |||
fromValue = fromMayToValue $ TString HasContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deserves a proper discussion. It seems that in some places the code was relying on ToString instance to generate strings with no context.
This changes the error message to reflect that strings with context are accepted.
Now, if the code was really relying on that assumption, it is still broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TString
? Or string class hub ToString
from relude
. In the second case - so far I just worked on strings that already were spawned in a regular way.
If to preserve context, or not, - I have no idea.
If the context is beneficial - of course, it should be preserved.
If there are really literal creations of the strings and they better to have context - then lets do it.
Personally, I am bothered & occupied with that we have String, strings of text of different data types, Nix String as a type of expression, Expression, StringContext, NixString. It seems for me that some of those need to be renamed/abstracted better.
===
I'd asked maybe a straight literal stream of consciousness of what you were doing, checking, understood, and found out, and what is your decision on it.
===
Lets preserve context where it seems to be in place, it would be a bit more memory footprint, but we so far not made solid space profiling. We always can drop the context.
@@ -205,7 +205,7 @@ instance ( Convertible e t f m | |||
(M.lookup "outPath" s) | |||
_ -> stub | |||
|
|||
fromValue = fromMayToValue $ TString NoContext | |||
fromValue = fromMayToValue $ TString HasContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TString
? Or string class hub ToString
from relude
. In the second case - so far I just worked on strings that already were spawned in a regular way.
If to preserve context, or not, - I have no idea.
If the context is beneficial - of course, it should be preserved.
If there are really literal creations of the strings and they better to have context - then lets do it.
Personally, I am bothered & occupied with that we have String, strings of text of different data types, Nix String as a type of expression, Expression, StringContext, NixString. It seems for me that some of those need to be renamed/abstracted better.
===
I'd asked maybe a straight literal stream of consciousness of what you were doing, checking, understood, and found out, and what is your decision on it.
===
Lets preserve context where it seems to be in place, it would be a bit more memory footprint, but we so far not made solid space profiling. We always can drop the context.
Maybe I did a tiny bad thing merging Is it ready yet? It seems a lot of discussion for the detail. Generally - if there are errors - there is no way to control them if there are no tests for that. Without the tests, the general good suggestion is to have a healthy sense of causality & things. Generally, we need to go with a healthy sense & ramp-up the testing suite workflow. |
This PR is as much a question as an answer. We use the FromValue stuff to cast string-like things to strings. It is used to extract antiquotations in attr selectors ( This PR is thus mergeable as-is. It moslty fixes the error message to make it consistent with the actual code. I think it can be merged, and yes, test would be a good thing :-) |
Function instances need type signatures to "undiscombabulate" themselves. I feel like it is again the same situation as you put previously. Maybe: the conversion instances logic can be expanded to pattern match & account/filter cases to transfer the context, and where there would be no context. What I know - is that I am pretty glad with the result of lifting up the |
In these, where either solution is partial & may produce bugs, good practice is to leave a note, at least for oneself in the future. A note to arrive to. |
I've noticed the solution to the: #958 (comment) |
No description provided.