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

Adds version to about/more and API #2181

Merged
merged 4 commits into from
Apr 21, 2017
Merged

Conversation

ashfurrow
Copy link
Contributor

@ashfurrow ashfurrow commented Apr 20, 2017

This adds a new lib/version.rb with a new VERSION constant string. This string is returned in /api/v1/instances as well as displayed on the about/more page.

(Edit: new screenshot.)

screen shot 2017-04-19 at 10 33 18 pm

screen shot 2017-04-19 at 8 27 23 pm

I'm still pretty new to this, so please feel free to point out anything that could be improved.

Fixes #1789.

@csu
Copy link
Contributor

csu commented Apr 20, 2017

It might make more sense to add this to the existing "Get current instance information" API endpoint (https://github.com/tootsuite/documentation/blob/master/Using-the-API/API.md#instances) than to create a new one just for the version. Other than that, looks good!

@ashfurrow
Copy link
Contributor Author

Makes sense to me, I'll update 👍

@badosu
Copy link

badosu commented Apr 20, 2017

Wouldn't it be sensible to use lib/mastodon/version.rb?

@ashfurrow
Copy link
Contributor Author

I mean, yeah that would make sense too. There's no current mastodon directory so this would be the only file in it. I wanted to keep it near the top of the directory structure because it is fairly important. But I'm open to moving it.

@badosu
Copy link

badosu commented Apr 20, 2017

I thought of this thinking of gem and namespace conventions, this is just a nitpick.

@ashfurrow
Copy link
Contributor Author

Gotcha. I'm happy to move it wherever, if that's more idiomatic.

@wxcafe wxcafe requested a review from mjankowski April 20, 2017 12:50
Copy link
Contributor

@mjankowski mjankowski left a comment

Choose a reason for hiding this comment

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

I agree w/ moving it to lib/mastodon/version.rb.

I had one other comment about using the constant in the views, otherwise looks fine.

.panel
.panel-header= t 'about.version'
.panel-body
%strong= Mastodon::VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor point, but as a general rule I try to avoid using constants directly in views ... can you wrap this up in InstancePresenter (view it to see how other values are already doing this), and then pass that in to the partial (see how contact is already doing that)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will do 👍

@ashfurrow
Copy link
Contributor Author

ashfurrow commented Apr 20, 2017

Okay, thanks @mjankowski and @badosu for the feedback, I've applied those changes and tested locally, let me know how it looks 👍

@mjankowski
Copy link
Contributor

This looks fine to me - I will defer to @Gargron on final approval here, because he's the one who will have to remember to change that number when he tags releases.

@ashfurrow
Copy link
Contributor Author

Makes sense!

@Gargron Gargron merged commit a0ed88a into mastodon:master Apr 21, 2017
Gargron pushed a commit that referenced this pull request Apr 23, 2017
@ashfurrow ashfurrow deleted the version branch April 26, 2017 07:55
seefood pushed a commit to Toootim/mastodon that referenced this pull request Apr 26, 2017
seefood pushed a commit to Toootim/mastodon that referenced this pull request Apr 28, 2017
* Adds version.

* Cleans up code.

* Removes standalone endpoint and adds version to instance endpoint.

* Addresses feedback from mastodon#2181.
seefood pushed a commit to Toootim/mastodon that referenced this pull request Apr 28, 2017
mkody pushed a commit to im-in-space/mastodon that referenced this pull request Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants