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

Fix python datetime functions #3048

Merged
merged 9 commits into from
Jun 6, 2018

Conversation

meold
Copy link
Contributor

@meold meold commented Jun 5, 2018

No description provided.

@kroitor kroitor self-assigned this Jun 5, 2018
@frosty00
Copy link
Member

frosty00 commented Jun 5, 2018

Is it worth rewriting these tests in js and transpiling with the new general transpiller transpileJavaScriptToPythonAndPHP?

@kroitor
Copy link
Member

kroitor commented Jun 5, 2018

Is it worth rewriting these tests in js and transpiling with the new general transpiller transpileJavaScriptToPythonAndPHP?

Yep, the ultimate goal is to unify the tests as well, one day... some day... )) These tests seem to mirror each other line-to-line, so I guess we will do something similar to what we did with precision tests. Feel free to do this or just let me know if you want me to unify them.

@meold
Copy link
Contributor Author

meold commented Jun 5, 2018

@kroitor and what about my PR?

@kroitor
Copy link
Member

kroitor commented Jun 5, 2018

@meold first of, thanks for the contribution. I'm watching the edits you make, good job so far! ) Now Travis says that it cannot divide the timestamp by a thousand in another method, where it previously could. This means that the new version of parse8601 function is now returning None for cases where it survived previously, compared to the earlier version of parse8601. Something was added that is making an almost-parseable timestring into a None now. And it didn't do this earlier. So, this makes me think, it's probably due to one of the newly added sanitizing clauses. Can you debug this further to make it pass, plz? ) Thx! )

@kroitor
Copy link
Member

kroitor commented Jun 5, 2018

@meold I mean, https://travis-ci.org/ccxt/ccxt/builds/388410386#L1776 ← multiple occurrences of this:

FAILED therock (Python 2): (explain @ run-tests.js:170)
	<type 'exceptions.TypeError'> unsupported operand type(s) for /: 'NoneType' and 'int' 
	
	  File "python/test/test.py", line 398, in <module>
	    main()
	
	  File "python/test/test.py", line 385, in main
	    try_all_proxies(exchange, proxies)
	
	  File "python/test/test.py", line 320, in try_all_proxies
	    test_exchange(exchange)
	
	  File "python/test/test.py", line 258, in test_exchange
	    test_symbol(exchange, symbol)
	
	  File "python/test/test.py", line 222, in test_symbol
	    test_ohlcv(exchange, symbol)
	
	  File "python/test/test.py", line 140, in test_ohlcv
	    ohlcvs = exchange.fetch_ohlcv(symbol)
	
	  File "/home/travis/build/ccxt/ccxt/python/ccxt/base/exchange.py", line 1103, in fetch_ohlcv
	    return self.build_ohlcv(trades, timeframe, since, limit)
	
	  File "/home/travis/build/ccxt/ccxt/python/ccxt/base/exchange.py", line 1146, in build_ohlcv
	    opening_time = int(math.floor(trade['timestamp'] / ms) * ms)  # Shift the edge of the m/h/d (but not M) (explain @ run-tests.js:173)

@frosty00
Copy link
Member

frosty00 commented Jun 5, 2018

@kroitor I'll do this, when I have time 0;

Also I don't like this style of coding:

except (TypeError, OverflowError, OSError, ValueError, AttributeError):
     return None

@frosty00
Copy link
Member

frosty00 commented Jun 5, 2018

@kroitor by unifying the tests do you mean all the tests or just a subset. I don't think all tests need to be unified, for example testing the exchange API responses can be done in just one language - I think we only do it in python and js but correct me if I'm wrong.

@kroitor
Copy link
Member

kroitor commented Jun 5, 2018

by unifying the tests do you mean all the tests or just a subset.

I mean a subset first, of course, but, will see how it goes, who knows... ))

@kroitor
Copy link
Member

kroitor commented Jun 5, 2018

Also I don't like this style of coding:

Ok, what would you propose instead of that line?

@frosty00
Copy link
Member

frosty00 commented Jun 5, 2018

@kroitor for example instead of:

except AttributeError:

if match is None:
    return

It makes the code easier to read

@meold
Copy link
Contributor Author

meold commented Jun 6, 2018

@kroitor fixed and now Travis is happy.

@kroitor kroitor merged commit f257568 into ccxt:master Jun 6, 2018
@meold meold deleted the fix_python_datetime_functions branch June 6, 2018 14:28
@frosty00
Copy link
Member

frosty00 commented Jun 6, 2018

@kroitor checkout #3058

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.

None yet

3 participants