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

Implement chart methods #8

Merged
merged 1 commit into from
Dec 1, 2016
Merged

Conversation

pfouque
Copy link

@pfouque pfouque commented Nov 30, 2016

I needed to use those methods which wasn't available in your module, so i did it.
BTW i'm a bit sorry about quality but this method is the only one to return array of different types and where relations are attached to a chart entity with id 0.
This time i deliver it directly with tests.

@browniebroke
Copy link
Owner

Thanks for the PR!

I'll look at it shortly. In the meantime, could you please merge/rebase my changes for coverages from the master branch? That would give more visibility on the status.

@codecov-io
Copy link

codecov-io commented Dec 1, 2016

Current coverage is 97.54% (diff: 100%)

Merging #8 into master will increase coverage by 0.29%

@@             master         #8   diff @@
==========================================
  Files             4          4          
  Lines           182        204    +22   
  Methods           0          0          
  Messages          0          0          
  Branches         20         22     +2   
==========================================
+ Hits            177        199    +22   
  Misses            3          3          
  Partials          2          2          

Powered by Codecov. Last update 8896af0...eaf22ce

@pfouque
Copy link
Author

pfouque commented Dec 1, 2016

It's done.
I also fixed code coverage by adding more tests on sub resources.
By doing so, i discovered a bug which is now fixed.

I'm not really satisfied with this implementation, especially the specific case in Client init, but as this API is really specific i didn't found another way to implement it in the actual base code without modifying core concept of your module.

So if you have another way to implement it i'm really curious to see how.

@browniebroke
Copy link
Owner

Thanks! Agree with you, but there are tests so we can improve it later if needed.

@browniebroke browniebroke merged commit 7ed9ca8 into browniebroke:master Dec 1, 2016
@browniebroke browniebroke added the enhancement New feature or request label Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants