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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show a weeks contributions from a given iso8601 date. #61

Merged
merged 1 commit into from Feb 13, 2015

Conversation

@kaspth
Copy link
Member

commented Feb 5, 2015

Hi Xavier!

On Fridays we send our weekly This Week in Rails newsletter and link to the top contributors.

Come Monday that link breaks because contributors/in-time-window/this-week has already moved on. We, however, are a bit more nostalgic and would like a canonical reference to a given week.

We also don't know when weeks begin and end - all I see anymore is newsletter Fridays. Ideally we'd like to input a date and have the app figure out which week it's part of.

I've added this feature including date range ballooning. Then by linking to contributors/in-week-of/2015-02-13 the coming Friday 13th we can really scare our readers 馃敧

cc @chancancode

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2015

Here's what it looks like (from the test environment):
skaermbillede 2015-02-05 kl 16 44 38

As you can see the text says "All Time". I'd like to fix this, but I wanted to hear your thoughts first.

@@ -1,5 +1,6 @@
RailsContributors::Application.routes.draw do
get 'contributors/in-time-window/:time_window' => 'contributors#in_time_window', as: 'contributors_in_time_window'
get 'contributors/in-week-of/:date' => 'contributors#in_week', as: 'contributors_in_week'

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Feb 5, 2015

Member

You could add a constraint to :date to ensure that the date is ISO 8601 format, e,g:

scope 'contributors', as: 'contributors', controller: 'contributors' do
  get 'in-time-window/:time_window', action: 'in_time_window', as: 'in_time_window'
  get 'in-week-of/:date', action: 'in_week', as: 'in_week', date: /\d{4}-\d{2}-\d{2}/
end

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 5, 2015

Author Member

鉂わ笍 this

Den 05/02/2015 kl. 16.46 skrev Andrew White notifications@github.com:

In config/routes.rb #61 (comment):

@@ -1,5 +1,6 @@
RailsContributors::Application.routes.draw do
get 'contributors/in-time-window/:time_window' => 'contributors#in_time_window', as: 'contributors_in_time_window'

  • get 'contributors/in-week-of/:date' => 'contributors#in_week', as: 'contributors_in_week'
    You could add a constraint to :date to ensure that the date is ISO 8601 format, e,g:

scope 'contributors', as: 'contributors', controller: 'contributors' do
get 'in-time-window/:time_window', action: 'in_time_window', as: 'in_time_window'
get 'in-week-of/:date', action: 'in_week', as: 'in_week', date: /\d{4}-\d{2}-\d{2}/
end

Reply to this email directly or view it on GitHub https://github.com/fxn/rails-contributors/pull/61/files#r24171776.

Kasper

@kaspth kaspth force-pushed the kaspth:permalink-week branch 2 times, most recently from 93842c9 to 6c13b1a Feb 5, 2015

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

Cool! Yes, let's do something about it.

Some feedback on the patch:

  1. In a routes file this simple, I'd prefer to keep the vanilla get call analogous to the existing one, the scope block does not seem to read better for my taste.

  2. The contrib app and all Rails docs are served by a small VPS. The secret is to have everything as static files. In the case of the contrib app via extended use of page caching, the patch would need to add that action to the macro call at the top (the existing expiration would already pick these new files).

@kaspth kaspth force-pushed the kaspth:permalink-week branch from 6c13b1a to f230713 Feb 6, 2015

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2015

Sweet, Xavier!

  1. I've removed the scope block.
  2. It's added.

I've also gone ahead and extended the commits controller to show week commits for a single contributor. This is helpful if we want to link to work that doesn't happen in a Pull Request like Sean's recent refactoring work.

I've changed the title that's displayed to no longer say "All Time".
skaermbillede 2015-02-06 kl 12 11 48
skaermbillede 2015-02-06 kl 12 12 01

It feels weird the sidebar highlights "All Time". We could insert an extra "Week" label above when a week is shown, but maybe that's excessive.

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

@pixeltrix the copy reads well? Since there's the range I am not 100% sure.

@pixeltrix

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

@fxn could be 'Week commencing 16 Jan 2012' - leave out the range since we know it's 7 days.

@kaspth maybe on the side navigation leave them all unhighlighted?

@kaspth kaspth force-pushed the kaspth:permalink-week branch from f230713 to ca5aa3d Feb 6, 2015

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2015

@pixeltrix I've changed it to "Week starting 16 Jan 2012", commencing was a bit too british 馃槃

Also the highlight is removed:
skaermbillede 2015-02-06 kl 19 27 57

Though the layout is a bit bonked. "See names mapping ->" is pushed down and the line height for the title is too small.

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

Also, it wraps.

Can we say "Week 14th of 2015" or something shorter?

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2015

If we strip "Rails" from "Rails Contributors" and format the week as "52nd week 2012" then it fits:
skaermbillede 2015-02-06 kl 21 17 30

I tried other combinations, but stripping Rails was the only thing that fit.

Though this doesn't prevent wrapping on people with longer names:
skaermbillede 2015-02-06 kl 21 18 05

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2015

This is the only thing that's close to fitting, when keeping Rails in, but it's not pretty:
skaermbillede 2015-02-06 kl 21 28 37

Show a weeks contributions from a given iso8601 date.
Adds a URL to see all contributions for the week of a given date.

So `contributors/in-week-of/2012-01-19` would show contributions between January 16th
and 22nd 2012.

@kaspth kaspth force-pushed the kaspth:permalink-week branch from ca5aa3d to 8c288e3 Feb 6, 2015

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 7, 2015

Let's give it some thought :).

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2015

I'm coming up dry here. Have you had any other ideas? :)

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 13, 2015

Do you know when is the newsletter going to be sent?

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2015

Today, but somewhere between 23-04 is the usual drill.

Kasper

Den 13/02/2015 kl. 13.48 skrev Xavier Noria notifications@github.com:

Do you know when is the newsletter going to be sent?


Reply to this email directly or view it on GitHub.

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 13, 2015

Let's do something. When I get home I'll merge and play with the headers. If I come up with something I'll deploy.

Let's coordinate with the newsletter to see whether the link is up or not, and use the old link if not. Sounds good?

/cc @chancancode

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2015

Sounds great.

Kasper

Den 13/02/2015 kl. 14.01 skrev Xavier Noria notifications@github.com:

Let's do something. When I get home I'll merge and play with the headers. If I come up with something I'll deploy.

Let's coordinate with the newsletter to see whether the link is up or not, and use the old link if not. Sounds good?

/cc @chancancode


Reply to this email directly or view it on GitHub.

fxn added a commit that referenced this pull request Feb 13, 2015
Merge pull request #61 from kaspth/permalink-week
Show a weeks contributions from a given iso8601 date.

@fxn fxn merged commit e5501dc into rails:master Feb 13, 2015

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 13, 2015

Once merged and used, I am not sure this is what the newsletter needs because it covers contributions from Friday to Friday. I wonder if a generic date range would make more sense for that use case.

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 13, 2015

Yep, won't deploy today, needs more thought I believe, perhaps I implement :from, :to or something.

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2015

Sounds fair 馃憤

But then the titles will be even longer 馃榿

Kasper

Den 13/02/2015 kl. 17.23 skrev Xavier Noria notifications@github.com:

Yep, won't deploy today, needs more thought I believe, perhaps I implement :from, :to or something.


Reply to this email directly or view it on GitHub.

@fxn

This comment has been minimized.

Copy link
Member

commented Feb 27, 2015

Hey, heads up to say this February has been frantic and have not made any progress here yet. But I eventually will :).

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2015

No worries. We're happy to wait to get it right :)

Kasper

Den 27/02/2015 kl. 12.59 skrev Xavier Noria notifications@github.com:

Hey, heads up to say this February has been frantic and have not made any progress here yet. But I eventually will :).


Reply to this email directly or view it on GitHub.

@fxn

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

@chancancode what do you think about this feature? As is, the newsletter in principle seems to need two things basically:

  • Be able to link to the time period the newsletter is covering, normally 7 days after some Friday.
  • The number of contributors mentioned should not vary. If the post says "36", the linked page ideally should not list 37 because more people committed in Friday after the post was publsihed.

Day ranges do not seem to be enough, maybe the time window should be expressed with timestamps? Would you guys use timestamps, use as lower limit the upper limit of the last post, etc.? It feels a bit too detailed as a first impression, but I don't see how can we support the points above without being more specific in the range.

Alternatively, we could revisit the two points above and see if there's maybe a judo approach that makes this less specific.

What do you think?

fxn added a commit that referenced this pull request Mar 2, 2015
Revert "Merge pull request #61 from kaspth/permalink-week"
Reason: I merged this patch because the PR looked good modulo details
I wanted to polish locally. But once the route was used locally I
realized we need to think a little bit more about it.

Since we cannot deploy master with this, and there is a new name
alias waiting to be deployed, we are going to revert by now.

This reverts commit e5501dc, reversing
changes made to 36432d7.
@kaspth

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2015

Timestamps sounds like a hassle, so 馃憤 for a judo approach.

What I was going for here was extracting specific timestamps from the given day. So maybe we just need to find out what we can expand and set a given parameter to? And what the parameter should be.

@fxn

This comment has been minimized.

Copy link
Member

commented Mar 8, 2015

@kaspth What do you mean? Can you put an example?

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2015

Well, my brain is a little musty here. I didn't extract specific timestamps but just expand a date in a week to the whole week.

So Friday 2015-13-02 would use 2015-02-09 and 2015-02-15 as the range.

Maybe we could add to that and find a date range expansion we could be happy with. Perhaps even pinning the dates to a specific time using for example end_of_day and friends.

@fxn

This comment has been minimized.

Copy link
Member

commented Mar 14, 2015

I see two cons in that approach:

  • It could happen (and I think it has happened), that the newsletter doesn't get out some week. A conference... Real Life... you know.
  • Since the newsletter is out in Friday and it may include stuff committed in that Friday, next day the committers are going to be more people (more are going to be included if there are more commits), and it could be the case that the newsletter of next week wants to include stuff committed last Friday after the newsletter was out, but that Friday can't be included anymore.

One possible solution would be:

  • There is an endpoint for a date range that defaults to a week for convenience.
  • People doing the newsletter have the discipline of not covering the day (UTC) the newsletter is being published. If it gets out on Friday, it links to the week from last Friday, to yesterday (Thursday), for example. That way the covered work and the committers count is going to be consistent in time (mod new name aliases).

What do you guys think?

@fxn

This comment has been minimized.

Copy link
Member

commented Mar 17, 2015

I am already implementing this solution.

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2015

Does your solution still allows us to cover a few things that's released on a friday? Just a few fridays ago there was a new rc release which we'd love to be able to include in the newsletter.

@fxn

This comment has been minimized.

Copy link
Member

commented Mar 18, 2015

Yep :), I am writing support for a date/timestamp range. It has to be contiguous (no gaps), but it is arbitrary.

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2015

Sounds great and thanks for helping us out 馃槃

@fxn

This comment has been minimized.

Copy link
Member

commented Mar 19, 2015

Hey just a ping to say it won't be available tomorrow yet, so that you know the newsletter can be out normally without waiting for this patch.

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2015

I don't remember you giving a timeframe, so I didn't expect tomorrow. But thanks for pinging :)

fxn added a commit that referenced this pull request Mar 21, 2015
implements an endpoint for date ranges
This patch adds an endpoint for date ranges. Examples:

    # All commits since August 2014
    http://contributors.rubyonrails.org/in-time-window/20140801

    # All commits in the first quarter of 2015
    http://contributors.rubyonrails.org/in-time-window/20150101-20150331

Timestamps can also be passed by adding hours, minutes, or seconds. Executive
summary: Time.zone.parse handles it.

As usual, everything is interpreted to be in UTC.

These endpoints are thought for the newsletter, there is no UI.

Thanks to @kaspth for his initial work towards these feature.

References #61.
@fxn

This comment has been minimized.

Copy link
Member

commented Mar 21, 2015

Arrgghh, the URLs in the commit message lack a "contributors" segment. They actually look like http://contributors.rubyonrails.org/contributors/in-time-window/20150314-20150321.

If a date (or timestamp) is passed instead of a range, the other endpoint is assumed to be now. From the point of view of the feature now seemed to have more sense than an arbitrary range of one week.

@chancancode

This comment has been minimized.

Copy link
Member

commented Mar 22, 2015

woooo. awesome. What's the default time(zone) for beginning/end-of-day if we don't specify?


Sent from Mailbox

On Fri, Mar 20, 2015 at 8:22 PM, Xavier Noria notifications@github.com
wrote:

Arrgghh, the URLs in the commit message lack a "contributors" segment. They actually look like http://contributors.rubyonrails.org/contributors/in-time-window/20150314-20150321.

If a date (or timestamp) is passed instead of a range, the other endpoint is assumed to be now. From the point of view of the feature now seemed to have more sense than an arbitrary range of one week.

Reply to this email directly or view it on GitHub:
#61 (comment)

@kaspth kaspth deleted the kaspth:permalink-week branch Mar 22, 2015

@fxn

This comment has been minimized.

Copy link
Member

commented Mar 22, 2015

Everything interpreted as UTC :). I'll write a post in Basecamp to let all
the team know.

El domingo, 22 de marzo de 2015, Godfrey Chan notifications@github.com
escribi贸:

woooo. awesome. What's the default time(zone) for beginning/end-of-day if
we don't specify?


Sent from Mailbox

On Fri, Mar 20, 2015 at 8:22 PM, Xavier Noria <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

Arrgghh, the URLs in the commit message lack a "contributors" segment.
They actually look like
http://contributors.rubyonrails.org/contributors/in-time-window/20150314-20150321
.
If a date (or timestamp) is passed instead of a range, the other
endpoint is assumed to be now. From the point of view of the feature

now seemed to have more sense than an arbitrary range of one week.

Reply to this email directly or view it on GitHub:
#61 (comment)


Reply to this email directly or view it on GitHub
#61 (comment).

@kaspth

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2015

This looks great, Xavier 鉂わ笍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.