search

andreroggeri / ynab-sdk-python

Python implementation of the YNAB API (https://api.youneedabudget.com/)
Apache License 2.0
29 stars 10 forks source link

Various fixes #74

Closed Tommaso-n closed 4 years ago

Tommaso-n commented 4 years ago

Renamed various fields as they shadowed builtin methods. Bumped version to 0.2.2

codeclimate[bot] commented 4 years ago

Code Climate has analyzed commit f12cea71 and detected 0 issues on this pull request.

View more on Code Climate.

codecov-commenter commented 4 years ago

Codecov Report

Merging #74 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #74   +/-   ##
=======================================
  Coverage   96.86%   96.86%           
=======================================
  Files          26       26           
  Lines        1021     1021           
  Branches       57       57           
=======================================
  Hits          989      989           
  Misses         30       30           
  Partials        2        2
Impacted Files Coverage Δ
ynab_sdk/api/models/responses/account.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/accounts.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/budget_detail.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/budget_settings.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/budget_summary.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/categories.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/category.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/payee.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/payees.py 100.00% <100.00%> (ø)
ynab_sdk/api/models/responses/transactions.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7601ba1...f12cea7. Read the comment docs.

andreroggeri commented 4 years ago

Hey @Tommaso-n please wait for review before merging PRs.

Why this change was needed ?

Tommaso-n commented 4 years ago

Ok, sorry. I'm going to write all the missing requests and responses and as i was checking the existing code, the linter reported that many field names were overwriting existing methods. I've simply renamed them

andreroggeri commented 4 years ago

I see, but you dont need to change the property name, just the method implementation becase it can shadow the native methods like id and format.

image

If we change just the inner impementation (Where it should be changed) there will be no breaking change for the library users.

Tommaso-n commented 4 years ago

I see. I will modify the code as you explained to me along with adding the missing classes

Tommaso-n commented 4 years ago

Before I make any more unintentional mistakes, is there any reason why some classes are duplicated? For example the Account class in account.py and accounts.py. Can I define them in one script and import them into the other?

andreroggeri commented 4 years ago

Yes, we should avoid to break the compatibility with our users.

Right now we still on pre-release versioning, but I think this change is unnecessary and will break for some users. We can still change the internal implementation where the IDE complains about shadowing.

Em ter, 25 de ago de 2020 19:01, Tommaso-n notifications@github.com escreveu:

Before I make any more unintentional mistakes, is there any reason why some classes are duplicated? For example the Account class in account.py and accounts.py. Can I define them in one script and import them into the other?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/andreroggeri/ynab-sdk-python/pull/74#issuecomment-680290973, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPRUYPMPRYHTIEVDFYXFA3SCQYCHANCNFSM4QKAUKXA .