-
Notifications
You must be signed in to change notification settings - Fork 225
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
feat: Add display
option to compile
, allow specifying colors
#4333
Conversation
Ok(prql_query) | ||
.and_then(prqlc_lib::prql_to_pl) | ||
.and_then(prqlc_lib::pl_to_rq) | ||
.and_then(|rq| prqlc_lib::rq_to_sql(rq, &options.unwrap_or_default())) | ||
.map_err(|e| e.composed(&prql_query.into())) | ||
.map_err(|e| (PyErr::new::<exceptions::PySyntaxError, _>(e.to_string()))) | ||
.map_err(|e| (PyErr::new::<exceptions::PyValueError, _>(e.to_string()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to ask this now, but is the display
argument really effective here?
I had trouble getting this option to work when I ported it to prqlc-r (PRQL/prqlc-r#317), but after looking at the source code I realized that this only works within the prql::compile
function.
IIUC, this function should be updated to something like:
prqlc_lib::compile(prql_query, &options)
.map_err(|e| PyErr::new::<exceptions::PyValueError, _>(e.to_string())))
(Sorry don't have time to try today, but if no one sees this I guess I can take a look in a few days)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm not sure why I did it that way, I guess I was adjusting from the previous version. Please feel free to simplify!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.