paurkedal / ocaml-caqti

Cooperative-threaded access to relational data
https://paurkedal.github.io/ocaml-caqti/index.html
GNU Lesser General Public License v3.0
299 stars 36 forks source link

Rename pretty-printers related to qprintf. #108

Closed paurkedal closed 1 year ago

paurkedal commented 1 year ago

@bclement-ocp I'd like to do this renaming to make the pretty-printers associated with qprintf less intrusive, or do you have another suggestion? I separate module could also work, though I think the pp_* prefix is fairly commonly used for this. As you can see, I'll leave the old names as deprecated for now. Sorry I didn't catch this while reviewing #103.

bclement-ocp commented 1 year ago

Ah, this is my bad actually, I wrote the code in a separate Caqti_fmt module initially, but I felt the name was confusing as it loosely implies functions for interacting with "regular" printf functions so I moved them to Caqti_query and should have updated the names.

I have the same qualms about using pp_param etc. -- the pp_* prefix is commonly used for interacting with regular Format-functions and I think it could be confusing. What about qp_* (short for Query Print rather than Pretty Print)? It makes it clearer that these functions are intended for use with qprintf.

paurkedal commented 1 year ago

I think we're thinking along the same lines here. When accepting the PR, it was already in the back of my mind that it broke one of my informal rules, namely that the fully-qualified identifier should fully identify what a definition is about. In the current version, we have "query" but no "format". My alternative would have been Caqti_query_format, which could as well be by Caqti_query_fmt, but, as you point out, not Caqti_fmt.

About pp_ vs qp_ I agree "pretty-print" is a misnomer. The pp_ prefix is used for more than pritty-printing already, but since these won't work with plain string output at all, and the types are insufficient to catch this, I agree.

Do you have a preference between qp_ prefixes and a separate Caqti_query_fmt module? The latter means that the constructors won't be immediately available in a local open for the query case, though the compiler should be able to find them based on the type (not tested yet).

bclement-ocp commented 1 year ago

Yes, I think we mostly agree here.

About pp_ vs qp_ I agree "pretty-print" is a misnomer. The pp_ prefix is used for more than pritty-printing already, but since these won't work with plain string output at all, and the types are insufficient to catch this, I agree.

As an aside: I thought about trying to make them do something sensible with string outputs, but I believe Q would be hopelessly broken anyways, so it being always broken sounded better.

Do you have a preference between qp_ prefixes and a separate Caqti_query_fmt module? The latter means that the constructors won't be immediately available in a local open for the query case, though the compiler should be able to find them based on the type (not tested yet).

I think my preference goes to qp_ in Caqti_query because Caqti_query is still a fairly small file and Caqti_query_fmt starts being a mouthful, but both are good options in my opinion.

paurkedal commented 1 year ago

I can confirm that lookups work with qprintf, so 5624f7d5c76ae4d0a3802db0900e33fe59246d72 would be an alternative.

I think my preference goes to qp_ in Caqti_query because Caqti_query is still a fairly small file and Caqti_query_fmt starts being a mouthful, but both are good options in my opinion.

Actually, from a maintenance point of view (if that's what you mean), I slightly prefer a separate module, since this is strictly built upon the public interface of Caqti_query with no need to smuggle in private definitions. So, it's a way ascertaining contributors that they can work on one without knowing much about the other.

Added: Well, since we'll keep deprecated aliases, the code will still be in Caqti_query for now the time being.

paurkedal commented 1 year ago

Final version 6fb63ca5e713bd43b3ebb47d86c161e33ef3ed13 (separate module).