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

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

Merged
merged 1 commit into from Jul 19, 2016

Conversation

Projects
None yet
3 participants
@hairyhenderson
Contributor

hairyhenderson commented May 20, 2016

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

Tumblr: Fixing double-read and off-by-one error
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

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 14, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@hairyhenderson

hairyhenderson Jul 14, 2016

Contributor

@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 😉...

Contributor

hairyhenderson commented Jul 14, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 15, 2016

Member

@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. 😄

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

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 19, 2016

Member

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

Member

parkr commented Jul 19, 2016

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

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 19, 2016

Member

@jekyllbot: merge

Member

parkr commented Jul 19, 2016

@jekyllbot: merge

@jekyllbot jekyllbot merged commit 14f19d1 into jekyll:master Jul 19, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyll/lgtm Approved by @parkr.

jekyllbot added a commit that referenced this pull request Jul 19, 2016

parkr added a commit that referenced this pull request Jul 19, 2016

tumblr: add test for JSON extraction
This new method was added in #253. This commit extracts the update into a
new method and adds a test for it.
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 19, 2016

Member

@hairyhenderson Added a test for this in 28f0cad.

Member

parkr commented Jul 19, 2016

@hairyhenderson Added a test for this in 28f0cad.

@hairyhenderson hairyhenderson deleted the hairyhenderson:fix-tumblr-import branch Jul 19, 2016

@hairyhenderson

This comment has been minimized.

Show comment
Hide comment
@hairyhenderson

hairyhenderson Jul 19, 2016

Contributor

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

Contributor

hairyhenderson commented Jul 19, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment