Skip to content

Tumblr: Fixing double-read and off-by-one error#253

Merged
jekyllbot merged 1 commit intojekyll:masterfrom
hairyhenderson:fix-tumblr-import
Jul 19, 2016
Merged

Tumblr: Fixing double-read and off-by-one error#253
jekyllbot merged 1 commit intojekyll:masterfrom
hairyhenderson:fix-tumblr-import

Conversation

@hairyhenderson
Copy link
Contributor

Found these bugs after I ran into #252

Fixing two bugs introduced recently:

  • feed was getting read from twice, so the second time it was empty
  • ending marks the index of the right-most }, but needed to be
    bumped by 1 when referenced in the array

Signed-off-by: Dave Henderson dhenderson@gmail.com

Fixing two bugs introduced recently:
- `feed` was getting read from twice, so the second time it was empty
- `ending` marks the index of the right-most `}`, but needed to be
  bumped by 1 when referenced in the array

Signed-off-by: Dave Henderson <dhenderson@gmail.com>
@parkr
Copy link
Member

parkr commented Jul 14, 2016

LGTM, thank you. Ideally this would have a test – do you have interest in creating one?

@parkr parkr added the fix label Jul 14, 2016
@hairyhenderson
Copy link
Contributor Author

@parkr - thanks! Yeah, I have interest in adding a test for this, but I'm not much of a Rubyist, so I'll have to bone up on the tests first probably 😉...

@parkr
Copy link
Member

parkr commented Jul 15, 2016

@hairyhenderson No problem! It'd probably come down to updating the test @jsonPayload to the new JSON output from that call (what you see there is old) and passing that input to this method so it can be parsed.

You might want to create a method self.extract_json which receives content and returns the value of blog:

# Parses the JSON from the JavaScript in the Tumblr API response
# and returns a Hash of its parsed data.
def self.extract_json(content)
  # ...
end

Then add a test which calls extract_json with the updated @jsonPayload variable and ensures you get the right thing in response. 😄

@parkr
Copy link
Member

parkr commented Jul 19, 2016

I'll add the test in a minute. 👍 LGTM.

@parkr
Copy link
Member

parkr commented Jul 19, 2016

@jekyllbot: merge

@jekyllbot jekyllbot merged commit 14f19d1 into jekyll:master Jul 19, 2016
jekyllbot added a commit that referenced this pull request Jul 19, 2016
parkr added a commit that referenced this pull request Jul 19, 2016
This new method was added in #253. This commit extracts the update into a
new method and adds a test for it.
@parkr
Copy link
Member

parkr commented Jul 19, 2016

@hairyhenderson Added a test for this in 28f0cad.

@hairyhenderson hairyhenderson deleted the fix-tumblr-import branch July 19, 2016 11:08
@hairyhenderson
Copy link
Contributor Author

Thanks @parkr - sorry for the lack of response! Was going to give it a shot, but stuff got in the way 😉

@jekyll jekyll locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants