-
Notifications
You must be signed in to change notification settings - Fork 323
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 Meta.engine_version
#11320
Add Meta.engine_version
#11320
Conversation
Output after changes to ordering:
|
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.
Yes this will be very useful :)
StringBuilder sb = new StringBuilder(); | ||
sb.append("Enso Engine Version: "); | ||
sb.append(BuildVersion.ensoVersion()); | ||
sb.append("\nDefault Edition: "); |
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 am surprised that on your Windows this displays newlines? Shouldn't you rather use System.lineSeparator()
?
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.
It works fine with \n
and we use it all over the place, e.g. in displaying the Test results. So I'd prefer to keep it as is.
Shouldn't this ultimately be part of the Project manager API? Isn't there an issue in GUI that tracks this? But yes, this seems useful, at least for the command line execution. |
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 don't like the place of the builtin, I don't like the API. Can we backout this PR, @radeusgd and start again?
## PRIVATE | ||
ADVANCED | ||
Returns the version of the currently running Enso engine. | ||
engine_version : Text |
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.
Please move the @Builltin_Method
into a private
module to be ready for to get ready for
delegate the builtin method here, if you want to expose it to users (even just with ## PRIVATE
comment).
|
||
@CompilerDirectives.TruffleBoundary | ||
private Text getCurrentVersion() { | ||
StringBuilder sb = new StringBuilder(); |
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 is not really an engine_version
. Engine version is 0.0.0-dev
- this is some string that needs to be parsed to get the engine version.
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.
Exposing text messages as an API is a well known antipattern which has bitten many. See for example Practical API Design, Chapter 3: "Determining what makes a good API", page 31:
Beware of situations where there is no alternative to parsing text messages! If the informa-
tion isn’t available in other ways, people will parse any textual output generated by your code.
For example, this happened to the JDK’s Exception.printStackTrace(OutputStream).
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.
Will you be ok if I rename this method to engine_version_info
or something 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.
The whole point of this method is to have a single method to get a quick glance of the currently running version.
Just 0.0.0-dev
version string is not enough to get that - it will be the same string for every dev build, regardless of which commit it was build from, so that tells very little.
If I split this up into separate methods, it will be tedious to reconstruct - the whole rationale is to easily get a debug overview.
This method is not meant to be used or parsed by users, if anyone uses it like that - I guess the usage is the anti-pattern. Is the ability for users to abuse a helper debug method enough reason not to have it?
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.
Is the ability for users to abuse a helper debug method enough reason not to have it?
Yes, it is! By exposing a text API like this either of:
- changing the text message
- removing the method
constitutes an incompatible change and it is a potential threat to existing Enso users.
In addition to that. In its current form this change doesn't meet requirements for an API change. There is a checkbox:
[ ] Unit tests have been written where possible.
Where else shall there be tests then when doing an API change?
Pull Request Description
Numerous times I wasn't sure when running the IDE if I'm running the bundled engine or a development build. Usually this depends on if I launch the standalone IDE or use a development build of project-manager.
Still it's not always obvious, and making sure that your IDE is running the right engine version is very often the first step when debugging issues with e.g. engine changes not showing up properly.
Thus I thought it may be worth to add this method (currently hidden to users in component browser by marking as
PRIVATE
, one has to type it in manually):I think it should be a helpful tool for debugging.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.