-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add configurable default target #4874
Conversation
This supports, for example, a compiler compiled for the x86_64-unknown-linux-musl architecture which doesn't require the --target commandline option to be set when ran on x86_64-unknown-linux-gnu. This is equivalent to the --target configure option in autoconf.
@@ -14,11 +14,13 @@ module Crystal | |||
|
|||
def self.description | |||
version, sha = version_and_sha | |||
if sha |
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.
I'd keep it as is or extract version string to another method.
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.
Version string is already a method. This is explicitly only used to print crystal -v
and isn't exposed anywhere else.
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.
Indeed, yet Crystal::Config.version
doesn't include sha and compile date making it less detailed.
Some examples of returned --version
output for other popular software:
- Ruby
ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-darwin15]
- Node.js
v8.1.2
- Rust
rustc 1.19.0
- Bundler
Bundler version 1.15.3
- Bower/Yarn/NPM/RubyGems
1.8.0
They're all one-liners.
Multi-line output is much less common, although it happens too:
- Java
java version "1.8.0_112" Java(TM) SE Runtime Environment (build 1.8.0_112-b16) Java HotSpot(TM) 64-Bit Server VM (build 25.112-b16, mixed mode)
- Swift
Apple Swift version 3.0.2 (swiftlang-800.0.63 clang-800.0.42.1) Target: x86_64-apple-macosx10.9
I'd still argue that oneline output is easier to read and parse, it also has wider usage rates.
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.
Nobody should be parsing --version
, it's purely for humans. As you can see, the message is composed from information which is available programatically from elsewhere in this class. I don't see the complaint.
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.
Multiline is fine. I would avoid an empty line and have Target:
thought.
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.
@ysbaddaden I prefer default target because it really is only the default. I think it's a rather pointless detail regardless.
This supports, for example, a compiler compiled for the
x86_64-unknown-linux-musl
architecture which doesn't require the--target
command-line option to be set when ran onx86_64-unknown-linux-gnu
. This is equivalent to the--target
configure option in autoconf.