Add buildinfo and version vars #1348
Conversation
Introduce `builtin:buildinfo` and `builtin:version` vars. This also changes the `-buildinfo -json` implementation to use the standard encoding/json package rather than handcrafting the JSON string. This also fixes three incorrect spellings of "overridden". Normally I would do those in a separate change but since there are only three instances, one of which one was legitimately part of this change, I decided to bundle the other two.
| if f.Version { | ||
| fmt.Fprintln(fds[1], fullVersion) | ||
| return nil | ||
| func GetBuildInfo() Buildinfo { |
There was a problem hiding this comment.
This needs to have a godoc, and it can be a simple var declaration instead of a function.
(If you're worried that the overridden values won't be picked up by a var declaration, those overrides happen at link time and directly modify the initial value of the variables.)
There was a problem hiding this comment.
Good point. This is a function because an earlier iteration of this change had more complicated logic in a misguided attempt to normalize the components of the full version string.
| // Note: The only way this will be executed is if option -buildinfo is also present. | ||
| // See the ShouldRun() method above. |
There was a problem hiding this comment.
// Note: The only way this will be executed is if option -buildinfo is also present.
// See the ShouldRun() method above.
That's not true? -json -version will also trigger this code path, and this PR changes the behavior from printing the version string to printing the entire buildinfo. Neither of them is correct; the correct behavior should be printing out the version string as a JSON-encoded string.
There was a problem hiding this comment.
The new implementation behaves the same as the old implementation. I don't think there is a good reason to have elvish -json -version emit the version as a JSON encoded string. Consider that doing so simply turns the value into a quoted string:
> put $version | to-json
"0.16.0-dev.bfb990f932611060edc198886396076004e48c53"
There was a problem hiding this comment.
Probably not very useful but consistent.
| can use commands like [`keys`](./builtin.html#keys) and | ||
| [`has-key`](./builtin.html#keys) on such objects. However, unlike a normal map | ||
| it is not possible to add new keys, remove existing keys, or even assign a new | ||
| value to an existing key. In other words, a pseudo-map is immutable. |
There was a problem hiding this comment.
In other words, a pseudo-map is immutable.
This is potentially misleading because normal maps are also immutable. The "mutation" operation supported by normal maps create copies.
There was a problem hiding this comment.
Yes, normal maps are also technically immutable but can be mutated using assoc explicitly or implicitly via magic LHS syntax. A pseudo-map cannot be mutated by assoc. That subtlety should probably be clarified but I couldn't see a way to word this that didn't hide the most important point of this paragraph.
There was a problem hiding this comment.
This concept is sometimes called "nondestructive mutation" but that concept would require its own paragraph to explain. The doc now says "it is currently not possible to create a modified version of an existing pseudo-map".
Introduce
builtin:buildinfoandbuiltin:versionvars. This alsochanges the
-buildinfo -jsonimplementation to use the standardencoding/json package rather than handcrafting the JSON string.
This also fixes three incorrect spellings of "overridden". Normally I would
do those in a separate change but since there are only three instances,
one of which one was legitimately part of this change, I decided to bundle
the other two.