KxSystems / rkdb

R client for kdb+
https://code.kx.com/q/interfaces
Apache License 2.0
41 stars 29 forks source link

Inconsistency of type conversions with the table in README #27

Closed lwshang closed 6 years ago

lwshang commented 6 years ago

Some entries in the type conversion table in README are wrong:

kdb/q r (wrong) r (correct)
timespan character difftime
time difftime integer

Correct means what actually happen in rkdb. Please check whether the README or the code should be fixed.

sv commented 6 years ago

Are you up for submitting a patch for readme?

On Mon, 9 Apr 2018, 11:55 pm Shang Linwei, notifications@github.com wrote:

Some entries in the type conversion table in README are wrong: kdb/q r (wrong) r (correct) timespan character difftime time difftime integer

Correct means what actually happen in rkdb. Please check whether the README or the code should be fixed.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/KxSystems/rkdb/issues/27, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA3BvYHp0YgV0wmZe9HADYJLULqHLktks5tm4RjgaJpZM4TMybq .

lwshang commented 6 years ago

I'm not sure whether we should just correct readme. Because it's reasonable to convert time to difftime, so maybe we should modify the from_time_kobject function to return a difftime object.

lwshang commented 6 years ago

Hi @sv ,

Are you considering this issue? Do you think it is better to change the way to deal with time instead of modifying readme?

I would like to fix it and make a pull request. But I want to have a consensus on it before start to working on it.

And could please to give some instructions on how to set up the develop tools for this project? I notice that you put the source codes into several separate files like common.c, base.c and sexp2k.c then include the header and these source files inside rkdb.c which will be compiled in the end. This is very different with my previous coding practice. What IDE or editor with plugin are you using to make the develop environment efficient?

sv commented 6 years ago

thanks for looking into submitting pull request. I think initially, we can just fix up documentation and discuss how we should handle time types consistently. Issue is that according to documentation where is no support for milliseconds in difftime(i might be wrong here) and we should not lose millis when converting. There also seem to be some consesus around bringing nanotime package into the mix.

for dev environment, i just use RStudio with devtools package and rarely need to move functions between files. Agree that common.c/base.c/sexp2k.c is a bit misleading and should be just header files that get imported in rkdb.c. We can cleanup this as we go

Let me know if you need any help with raising PR