PokeAPI / pokepy

A Python wrapper for PokéAPI
https://pokeapi.co
BSD 3-Clause "New" or "Revised" License
127 stars 27 forks source link

Fix fcache not handling different filesystems on Windows #55

Closed Kronopt closed 4 years ago

Kronopt commented 4 years ago

fix #54

Kronopt commented 4 years ago

Waiting for a review from leonardohra on #54

Naramsim commented 4 years ago

Hi @Kronopt, I've run the tests on Win10 py27 and I got only one error:

E.........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
======================================================================
ERROR: test_custom_cache_directory (tests.test_pokepy.TestV2Client)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests\test_pokepy.py", line 352, in test_custom_cache_directory
    client = pokepy.V2Client('in_disk', cache_dir)
  File "pokepy\api.py", line 131, in __init__
    super(V2Client, self).__init__(*args, **kwargs)
  File "C:\Python27\lib\site-packages\beckett\clients.py", line 195, in __init__
    self.assign_resources(self.Meta.resources)
  File "C:\Python27\lib\site-packages\beckett\clients.py", line 210, in assign_resources
    self.assign_methods(resource)
  File "C:\Python27\lib\site-packages\beckett\clients.py", line 226, in assign_methods
    method.upper()
  File "pokepy\api.py", line 165, in _assign_method
    resource=resource_class, data=None, **kwargs):
  File "pokepy\api.py", line 221, in memoize
    raise TypeError('expected str, not %s' % cache_directory.__class__.__name__)
TypeError: expected str, not unicode

----------------------------------------------------------------------
Ran 938 tests in 4.333s

FAILED (errors=1)
Naramsim commented 4 years ago

So this is the command I ran: py -m unittest tests.test_pokepy

For python 2.7.17 I got the error above, while for 3.7.6 I got none.

Kronopt commented 4 years ago

Nice catch. I'll look at it tomorrow

codecov[bot] commented 4 years ago

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #55   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files           3        3           
  Lines        1098     1098           
=======================================
  Hits         1096     1096           
  Misses          2        2           
Impacted Files Coverage Δ
pokepy/__init__.py 100.00% <100.00%> (ø)
pokepy/api.py 98.23% <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 abd00af...e6984ec. Read the comment docs.

Naramsim commented 4 years ago

All good now on Python 2.7 😄

Kronopt commented 4 years ago

I ported all of fcache's code to pokepy and implemented the fix. When the fix is implemented on the fcache repo, I'll undo this and get back to using fcache as a dependency. The test error you caught must have been because I bumped all python versions to their latest micro release on the circleci config file

Naramsim commented 4 years ago

You can go along and merge if you want, you can also propose your fix to the fcache library :)

Kronopt commented 4 years ago

I'll just fix one last thing, wait for more feedback from leonardohra on #54 and I'll merge. I also have already submitted a PR to the fcache library :)