Add API endpoint for fetching global track files #124

ramyaravindranath · 2016-08-09T05:59:38Z

No description provided.

Insti · 2016-08-09T06:50:42Z

Can you add some description to the commit message that gives us more information about this?
It should tells us what the endpoint is, why it is useful, references any issues/PRs that have further discussion.

Insti · 2016-08-09T06:52:44Z

test/xapi/track_test.rb

+      end
+    end
+    assert_equal filenames.sort, files.sort
+  end


I really like that you've extracted assert_archive_contents into a method. 👍

while (entry = io.get_next_entry) files << entry.name end

Is io Enumerable? Is it possible to replace this with io.map(&:name)?

That is already implemented. I have just reused this code :)

I have some suggestions about the naming here: assert_archive_contains(filenames, zip)

What do you think about:

assert_archive_filenames(zip, filenames)

'contains' can be ambiguous and it's not clear whether the method will check all or only some of the filenames, and if contains refers to the data that is zipped up.

The arguments to the method are in the same order as they are in the method name.

Turns out it's not Enumerable anyway. :(

The arguments to the method are in the same order as they are in the method name

Most assert methods use the ordering: expected, actual. What about assert_filenames_in_archive? Too clunky?

Most assert methods use the ordering: expected, actual. What about assert_filenames_in_archive? Too clunky?

You are quite right. assert_filenames_in_archive would be fine.

But the more I think about this the more I don't like that method at all.
Here is my reasoning:

The actual assertion we want is: assert_equal expected, actual

Where:
expected = known_filenames.
actual = filenames_in_the_archive.

Which gives us: assert_equal known_filenames, filenames_in_the_archive

We have known_filenames which is files.keys (line 115 below)

What we need is filenames_in_the_archive

Ideally, we would get this by sending the archive a filenames message, (archive.filenames) but since it doesn't respond to that, we can make a wrapper method: archive_filenames(archive) which will return an array of filenames.

Extracting that part from our custom assertion, gives us a new method:

def archive_filenames(zip) files = [] Zip::InputStream.open(zip) do |io| while (entry = io.get_next_entry) files << entry.name end end files end

Which I like because it is only doing one thing (getting the filenames from the archive) and leaves the assertion (a separate responsibility) as :
assert_equal known_filenames, archive_filenames(archive)

That done, we can remove the custom assert_archive_contains/assert_archive_filenames/assert_filenames_in_archive method completely.

ramyaravindranath · 2016-08-09T06:52:53Z

@Insti It says this branch has merge conflict. I will resolve it first and edit my commit message

Insti · 2016-08-09T12:48:04Z

Build is failing due to a Rubocop error:
lib/xapi/track.rb:6:3: C: Class has too many lines. [123/100]

Some things to think about:
Does the functionality you've added need to be a part of the Track class?
Could it be implemented in its own class?
Which other methods from the Track class does it depend on?

ramyaravindranath · 2016-08-09T12:50:41Z

@Insti:This is the PR for https://github.com/exercism/x-api/issues/29.

ramyaravindranath · 2016-08-09T12:54:47Z

@Insti I will think about it and update you. I am figuring how tests are written for routes on line 58. I will implement it for tracks as well

Insti · 2016-08-09T13:14:06Z

test/api/v3/routes/tracks_test.rb

+    assert_equal 200, last_response.status
+    assert_equal last_response.headers["Content-Disposition"].split(';')[0], 'attachment'
+    assert_equal last_response.headers["Content-Type"], 'application/octet-stream'
+  end


Insti · 2016-08-10T14:16:14Z

@ramyaravindranath Nice one on the class extraction. I'll have a closer look at it a bit later, but that was what I thought was needed. Good job.

Insti · 2016-08-10T18:23:44Z

api/v3/routes/tracks.rb

@@ -65,6 +65,14 @@ class Tracks < Core
        implementation.zip.string
      end

+      get '/tracks/:id/global' do |id|
+        track = find_track(id)
+        filename = "%s-%s.zip" % [id, "global"]


Is this line tested?
What would happen if I changed it to `filename = 'blablabla'?

@Insti Thanks for pointing it out. New test has been added.

Insti · 2016-08-10T19:18:27Z

Ok, I've looked through it more closely now, and am impressed by what you've done.

I've made a few suggestions for things I think are possible improvements, (but not because I think what you have is not good enough!) Have a think about them and see if you agree. It may be that you think that it is unnecessary work, and you're happy with the way things are, But if you've got more time and want to stretch yourself a bit I think they will make the code more elegant and easier to work with for the next person who comes along to work on it.

Overall, great work. ❤️

Moved large chunks of text into `Approvals` files. Remove redundant tests. Replace custom assertion with regular assertion + helper method. See discussion here: exercism#124 (comment) Simplified individual test methods. Flog score improvement: Old version: 69.3: flog total 8.7: flog/method average New version: 52.8: flog total 5.9: flog/method average

Insti · 2016-08-11T07:54:45Z

Those latest changes have made things much nicer.
Is there anything more you want to do before you call it done?

ramyaravindranath · 2016-08-11T11:15:15Z

@Insti : It is done for now 😄

Insti · 2016-08-11T13:22:04Z

Great, I'll tag it as ready and then @kytrinyx can look over it and merge it. 👍

kytrinyx · 2016-08-11T14:54:20Z

lib/xapi/file_bundle.rb

+      @ignore_patterns = ignore_patterns
+    end
+
+    def create_zip


I avoid names that say what a method does, and try to name things for what the method returns. This is because the implementation could change (we could decide to create the zip elsewhere, and this method would just return the zip that we had created).

kytrinyx · 2016-08-13T17:13:41Z

@ramyaravindranath This needs a couple of small tweaks before it can be merged.

@exercism/rgsoc2016 I've got some feedback with respect to pull requests in general.

The advice in this article about commit messages is also relevant to pull requests.

When you submit a pull request, try to make the title state the most important idea, and then in the body of the pull request, explain why this was necessary, and summarize the details.

Here the title says adds new api end point, which means that the reader has to figure out what endpoint and why. A better subject-line would be

Add API endpoint for fetching global track files

or

Implement support for exercise-agnostic files in tracks

If the pull request fixes an existing issue, then it's important to reference that, either in the subject or the body of the pull request—preferably in a way that would automatically close the issue when the pull request gets merged. See https://github.com/blog/1506-closing-issues-via-pull-requests for details.

Even though there is often a discussion about the change elsewhere, it's important to summarize the most important / most relevant points about the actual change that was made in the pull request description.

The goal is to let the reviewer have a solid idea of what the code is doing and why before actually looking at the code—and without going to read through other discussions elsewhere, unless they want more granularity.

Without it, the reviewer has to wade through an entire discussion where the details have been hashed out, often through dozens of comments, and they also have to read the code while guessing what the change is about. If the important changes are mentioned up front, then it's possible to review the patch much more strategically instead of top-to-bottom.

If you have a pull request that includes both refactoring and changes, then it's a huge help if you commit the refactoring in a separate git commit, before making the change. This lets the reviewer review the refactoring first without mixing in new behavior, and it also lets them look at the new behavior without mixing in the refactoring changes. That new behavior will be a smaller diff, and therefore easier to understand. Often I end up starting to make a change, and then realizing that I'm going to refactor. At that point I usually stash the changes that I've already made, refactor, commit, and unstash. Or sometimes I make a branch. This sometimes means that I have to redo some of the work that I already did, but it's worth it to have the commits be atomic.

ErikSchierboom · 2016-08-15T13:04:17Z

Excited to see it is almost done :)

kytrinyx · 2016-08-16T00:25:22Z

@ramyaravindranath Would you post a comment here when you're ready for the final review?

ramyaravindranath · 2016-08-16T01:50:30Z

@kytrinyx I have fixed all the comments. It is ready for the final review. Thank you.

kytrinyx · 2016-08-18T19:09:40Z

Thank you @ramyaravindranath! Merged in d1385d7

ErikSchierboom · 2016-08-28T17:55:23Z

@kytrinyx I think the CLI also has to be updated, right?

kytrinyx · 2016-09-02T14:37:01Z

Yepp: exercism/cli#88

kytrinyx · 2016-09-02T14:37:37Z

Oh, haha, you found that one :)

ErikSchierboom · 2016-09-02T15:11:46Z

I did 😀

Insti reviewed Aug 9, 2016
View reviewed changes

Insti reviewed Aug 10, 2016
View reviewed changes

Insti added the in progress label Aug 10, 2016

Insti mentioned this pull request Aug 10, 2016

Refactor implementation test #125

Merged

Insti added the ready label Aug 11, 2016

kytrinyx reviewed Aug 11, 2016
View reviewed changes

kytrinyx removed the ready label Aug 13, 2016

ramyaravindranath changed the title ~~adds new api end point~~ Add API endpoint for fetching global track files Aug 15, 2016

ramyaravindranath added 2 commits August 15, 2016 15:21

adds new api end point to fetch supporting files for the language track

a16e12c

Convert FileBundle to a class

503d57e

ramyaravindranath added 2 commits August 15, 2016 15:21

Changing create_zip to zip and list_paths to paths

9017d78

Adds test to check the name of the attachment

e7bf25f

kytrinyx pushed a commit that referenced this pull request Aug 18, 2016

Merge pull request #124 from TeamVegemite/adds-new-api-end-point

d1385d7

kytrinyx closed this Aug 18, 2016

Insti removed the in progress label Aug 18, 2016

This was referenced Aug 30, 2016

Add support for fetching files to root track dir exercism/cli#88

Closed

Using FsUnit for more expressive tests exercism/fsharp#203

Closed

kytrinyx mentioned this pull request Sep 8, 2016

IntelliJ vs Eclipse - where to document what? exercism/java#132

Closed

Insti mentioned this pull request Oct 9, 2016

timestamp_improvement, cover the case from 23.5 to 24 hours exercism/exercism#3159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API endpoint for fetching global track files #124

Add API endpoint for fetching global track files #124

ramyaravindranath commented Aug 9, 2016

Insti commented Aug 9, 2016

Insti Aug 9, 2016

Insti Aug 9, 2016

ramyaravindranath Aug 9, 2016

Insti Aug 9, 2016

Insti Aug 9, 2016

TimMoore Aug 10, 2016

Insti Aug 10, 2016 •

edited

Loading

ramyaravindranath commented Aug 9, 2016

Insti commented Aug 9, 2016

ramyaravindranath commented Aug 9, 2016

ramyaravindranath commented Aug 9, 2016

Insti Aug 9, 2016

Insti commented Aug 10, 2016

Insti Aug 10, 2016

ramyaravindranath Aug 15, 2016

Insti commented Aug 10, 2016

Insti commented Aug 11, 2016

ramyaravindranath commented Aug 11, 2016

Insti commented Aug 11, 2016

kytrinyx Aug 11, 2016

kytrinyx commented Aug 13, 2016

ErikSchierboom commented Aug 15, 2016

kytrinyx commented Aug 16, 2016

ramyaravindranath commented Aug 16, 2016

kytrinyx commented Aug 18, 2016

ErikSchierboom commented Aug 28, 2016

kytrinyx commented Sep 2, 2016

kytrinyx commented Sep 2, 2016

ErikSchierboom commented Sep 2, 2016

Add API endpoint for fetching global track files #124

Add API endpoint for fetching global track files #124

Conversation

ramyaravindranath commented Aug 9, 2016

Insti commented Aug 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

ramyaravindranath commented Aug 9, 2016

Insti commented Aug 9, 2016

ramyaravindranath commented Aug 9, 2016

ramyaravindranath commented Aug 9, 2016

Choose a reason for hiding this comment

Insti commented Aug 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti commented Aug 10, 2016

Insti commented Aug 11, 2016

ramyaravindranath commented Aug 11, 2016

Insti commented Aug 11, 2016

Choose a reason for hiding this comment

kytrinyx commented Aug 13, 2016

ErikSchierboom commented Aug 15, 2016

kytrinyx commented Aug 16, 2016

ramyaravindranath commented Aug 16, 2016

kytrinyx commented Aug 18, 2016

ErikSchierboom commented Aug 28, 2016

kytrinyx commented Sep 2, 2016

kytrinyx commented Sep 2, 2016

ErikSchierboom commented Sep 2, 2016

Insti Aug 10, 2016 •

edited

Loading