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

Rename pretty-printers related to qprintf. #108

Closed
wants to merge 1 commit into from

Conversation

paurkedal
Copy link
Owner

@paurkedal paurkedal commented Aug 29, 2023

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

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

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

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

paurkedal commented Sep 4, 2023

I can confirm that lookups work with qprintf, so 5624f7d 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
Copy link
Owner Author

Final version 6fb63ca (separate module).

@paurkedal paurkedal closed this Sep 5, 2023
@paurkedal paurkedal deleted the qprintf-renames branch September 5, 2023 07:31
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.

2 participants