search

eyecatchup / SEOstats

SEOstats is a powerful open source PHP library to request a bunch of SEO relevant metrics.
MIT License
1.46k stars 384 forks source link

Added methods to use Sistrix API #89

Closed tholu closed 9 years ago

tholu commented 9 years ago

Fixes issue #88.

If my other PR is merged, you can cherrypick the relevant commits from this PR. Thanks!

ClemensSahs commented 9 years ago

@tholu Thanks for your PR, I try my best to find time for a code-review

This PR has a commit overlap with the PR #87

For the next time please work with the general git workflow, so we can merge directly PR. "Only topic for on PR, make it easier." :smile:

Can you add Tests for that code?

tholu commented 9 years ago

I already committed the changes to local master, and putting the two last commits on their own branch obviously didn't help, since they depend on the previous commits from the other PR. I will branch before making changes next time, so multiple PRs are possible without overlapping.

Will look into the the Tests.

ClemensSahs commented 9 years ago

On a short code review, I have some little annotation for code climate.

That all looks good, after tests and the better code climate I will merge, soon as possible.

tholu commented 9 years ago

Thanks for your comments, I have commented as well. Everything seems straightforward, but can you take a look at the API guard method (see my description above)?

ClemensSahs commented 9 years ago

hehe thanks, I love to review and refactor codes. If you understand a function in 30s that code is great, if not you can start to the refactoring. This is a simple rule and one of the best, too. :smile:

I commented the guard method question above.

tholu commented 9 years ago

Thanks, I really appreciate it! I like beautiful code as well :)

tholu commented 9 years ago

I guess the latest build failed because I changed the original Sistrix VI method to return a real integer with a point as a decimal point instead of a comma (to be consistent with the API data). However this may break installations which rely on the previous behaviour. What should we do about that?