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

Allow specifying lang order independently of names #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wfdd
Copy link

@wfdd wfdd commented Jul 12, 2016

This moves a few things around in scrape_wikidata to make it possible
to use the lang parameter (renamed lang_precedence) when passing a
list of ids and no names hash. Creating the names hash would've
been shadowed by a non-nil lang on line 70.

This moves a few things around in `scrape_wikidata` to make it possible
to use the `lang` parameter (renamed `lang_precedence`) when passing a
list of `ids` and no `names` hash.  Creating the `names` hash would've
been shadowed by a non-nil `lang` on line 70.
@wfdd
Copy link
Author

wfdd commented Jul 12, 2016

I've flipped through all 17 pages of https://github.com/search?l=ruby&q=scrape_wikidata&type=Code&utf8=✓ and lang wasn't in any of the results, which is why I felt safe to rename the parameter.

Tests would probably be nice.

@tmtmtmtm
Copy link
Contributor

thanks @wfdd — at a team meeting yesterday and today, so haven't had a chance to look too deeply at this yet.

The logic is getting quite complex now, so, yes, I think tests are definitely high priority (and adding them would probably force us to refactor in useful ways so we can actually get at the things that are important to test)

@wfdd
Copy link
Author

wfdd commented Jul 14, 2016

Do you wanna mock the I/O methods or uncouple them entirely and only test the data munging bits? I've not seriously worked in Ruby before, but I could give it a shot.

On 14 Jul 2016, at 10:42, Tony Bowden notifications@github.com wrote:

thanks @wfdd — at a team meeting yesterday and today, so haven't had a chance to look too deeply at this yet.

The logic is getting quite complex now, so, yes, I think tests are definitely high priority (and adding them would probably force us to refactor in useful ways so we can actually get at the things that are important to test)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@tmtmtmtm
Copy link
Contributor

If I implemented it myself I'd probably split the parts out to separate method so we could test them independently, but that's not based on actually having looked into how that might work, so if you do have a go, feel free to do whatever is easiest for you.

FWIW, rather than mocking, we tend to just use the vcr gem to record actual results — though here I'd be more concerned with just making sure we're passing the right thing to Wikisnakker/ScraperWiki etc, and trust them to do the right things (or improve the testing of those parts elsewhere).

@wfdd
Copy link
Author

wfdd commented Jul 26, 2016

Probably not gonna have time to refactor this if you wanna close it or merge it anyway.

@tmtmtmtm tmtmtmtm self-assigned this Aug 15, 2016
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.

2 participants