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

Inconsistency of type conversions with the table in README #27

Closed
lwshang opened this issue Apr 9, 2018 · 4 comments · Fixed by #28
Closed

Inconsistency of type conversions with the table in README #27

lwshang opened this issue Apr 9, 2018 · 4 comments · Fixed by #28

Comments

@lwshang
Copy link
Contributor

lwshang commented Apr 9, 2018

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
Copy link
Contributor

sv commented Apr 9, 2018 via email

@lwshang
Copy link
Contributor Author

lwshang commented Apr 9, 2018

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
Copy link
Contributor Author

lwshang commented Apr 18, 2018

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
Copy link
Contributor

sv commented Apr 22, 2018

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

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 a pull request may close this issue.

2 participants