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

Fixed #12 #13

Merged
merged 1 commit into from
Jan 2, 2017
Merged

Fixed #12 #13

merged 1 commit into from
Jan 2, 2017

Conversation

larsgw
Copy link
Contributor

@larsgw larsgw commented Dec 31, 2016

It should fix #12 and I fixed the test cases to accommodate the API change. Compiling with coffee 1.12.2 instead of 1.11.1, which makes most of the changes. Perhaps the API change should be optional, but that's not for me to decide. I haven't updated the docs yet.

@larsgw
Copy link
Contributor Author

larsgw commented Jan 1, 2017

I overlooked some things with the qualifiers. I'll fix them as fast as possible:

  • qualifiers is not a part of mainsnak but part of claim
  • qualifiers seems to be different from normal claim collections, which is harder to fix

(Both fixed now)

@maxlath
Copy link
Owner

maxlath commented Jan 1, 2017

Thanks for the PR!
To make things simpler (hopefully), I have finally got into the long awaited task of converting the project from coffeescript to javascript \o/
Could you do your changes from this new version and update this PR? Please don't commit the build files, I do that before releasing to make it easy to get started with using the lib, but they should not pollute the list of file changes the rest of the time.

On the PR's content, from what I saw, I would prefer not breaking the current interface, and only get the qualifiers if a given option is passed, what do you think?

@larsgw
Copy link
Contributor Author

larsgw commented Jan 2, 2017

I changed it to work like this:

wdk.simplifyClaims(entity.claims, null, null, true)
wdk.simplifyPropertyClaims(entity.claims.P50, null, null, true)
wdk.simplifyClaim(entity.claims.P50[0], null, null, true)

@larsgw larsgw reopened this Jan 2, 2017
@maxlath maxlath merged commit 759235c into maxlath:master Jan 2, 2017
@maxlath
Copy link
Owner

maxlath commented Jan 2, 2017

thanks! I merged and made a few corrections:

  • added tests 988a641
  • formatted a few things to my taste ;) c86f692
  • fixed an issue with in your implementation: it wasn't including property prefixes in qualifiers claims 7c3e5fc (see tests)

Could you review those changes before I publish the new release?

@larsgw
Copy link
Contributor Author

larsgw commented Jan 2, 2017

Note: I only found the commenting-code-inline button when it was too late.

The tests seem good although for...in shouldn't be used for arrays. It's better to use for...of, and if you want indices use the following (see this):

for (let [index, value] of array.entries()) {
  // ...
}

Formatting is entirely you choice, of course. Line-by-line:

  • 40: Not my prefered style, but it's not my code :)
   const simpleQualifiers = {}
  • 71: I used let because the properties would change, but I forgot that that didn't matter
     .slice()
     .map(simplifyQualifierClaim)
  • 78: I did the whitespace the same way you used here
   const simplifyQualifierClaim = function (claim) {
     return simplifyClaim({ mainsnak: claim }, entityPrefix, propertyPrefix)
   }

and

     .map(simplifyQualifierClaim)
  • 79: You used inline arrow-functions here, so I thought it would be okay

Concerning the implementation issue: I changed from calling simplifyClaims() to simplifyClaim() because you need to loop over the properties anyway, and I forgot the prefix.

@maxlath
Copy link
Owner

maxlath commented Jan 2, 2017

I tend to write

return bar
.slice()
.map(buzz)

but add indentation when the result of the chain is assigned to some variable

var foo = bar
  .slice()
  .map(buzz)

I find it clearer, but some might find it inconsistent I guess ^^

maxlath added a commit that referenced this pull request Jan 2, 2017
@maxlath
Copy link
Owner

maxlath commented Jan 2, 2017

@larsgw I just released 4.2.0, thank you for your contribution \o/

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.

Add option to keep qualifiers in simplifyClaims()
2 participants