Skip to content
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 information on the About section of the client #497

Merged
merged 3 commits into from
Jul 16, 2016

Conversation

astorije
Copy link
Member

@astorije astorije commented Jul 13, 2016

This rewords current version string, adds useful links to the user, and displays whether the instance is running from a release or from the git repo.
This last piece will be very helpful to debug when users come asking help on the IRC channel.

From a release From source
screen shot 2016-07-13 at 03 03 48 screen shot 2016-07-13 at 03 03 19

@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jul 13, 2016
.toString()
.trim();
} catch (e) {
// Not a git repository or git is not installed: treat it as npm release
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If git is not installed, then we will falsely report that this is an npm release. However, if this is a git repo, chances are git is installed, and is available to the user running the instance.

Alternative is to check if there is a .git directory, but this is much more complex (not trivial to find the hash of current commit by just exploring the directory, very error-prone!). It is somewhat not 100% reliable as well as one can have a git repo without a .git folder (well, I didn't know), though this case must be super rare. But who knows, maybe some servers centralize .git folders, as some do with Python virtualenvs?

Another alternative is to use a JS implementation of git, such as https://github.com/creationix/js-git, but then we're talking über-overkill and another world of issues incoming.

To conclude: not the most reliable, but the simplest solution without a doubt, and because of the try/catch, it will not break a server for various and random reasons.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If git is not installed, then we will falsely report that this is an npm release.

Yeah, let's just ignore that. If someone has somehow got the git repository without having git installed, then we don't really care, as it's such a small thing anyway.

I think this solution is fine for what you are trying to do. The failure cases are so rare, and it's such a small thing (it'll just show it as a release rather than git) that it really doesn't matter.

@AlMcKinlay
Copy link
Member

I'm guessing your images are the wrong way round.

@astorije
Copy link
Member Author

I'm guessing your images are the wrong way round.

It was, thanks! 😅

@AlMcKinlay
Copy link
Member

Great, that makes much more sense. 👍 then

@maxpoulin64
Copy link
Member

Yep, looks all good to me. No need to go crazy about a JS implementation, reasonably safe to assume people running the git version have the git binary in their path. Worst case, it's the current behavior, so meh. 👍 and merging

@maxpoulin64 maxpoulin64 merged commit 001a3c5 into master Jul 16, 2016
@maxpoulin64 maxpoulin64 deleted the astorije/about-section branch July 16, 2016 18:52
@astorije astorije added this to the 2.0.0 milestone Jul 17, 2016
@dgw
Copy link
Contributor

dgw commented Aug 3, 2016

Is this supposed to support instances running after npm install -g from the source?

npm start = git reference; running with my init script shows 2.0.0-pre.4 (but has features added since that prerelease).

@maxpoulin64
Copy link
Member

@dgw: it tries to find if it is a git repo, and falls back to the version in package.json. If for any reason git fails, then it will display the npm version. My guess is that your initscript's working directory is not inside the repo, so git can't find it.

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Add information on the About section of the client
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants