Skip to content

Conversation

@takanuva15
Copy link
Contributor

@takanuva15 takanuva15 commented Feb 13, 2025

Add EscapeQueryChars util function based on solr docs. (I put it in a utils file but I can move it elsewhere if preferred)

closes #29

@takanuva15
Copy link
Contributor Author

@stevenferrer Hi, looks like the builds are all passing now. Request review when possible

@stevenferrer
Copy link
Owner

Hi @takanuva15, just to understand, the purpose of this utility function is to escape the especial characters in the query field, is that correct?

@takanuva15
Copy link
Contributor Author

Yes. To be accurate, this function is derived from the official Java client that the Solr team themselves publish on their GitHub repo here: https://github.com/apache/solr/blob/main/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java#L205

@stevenferrer
Copy link
Owner

Yes. To be accurate, this function is derived from the official Java client that the Solr team themselves publish on their GitHub repo here: https://github.com/apache/solr/blob/main/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java#L205

Since this is considered a utility function, maybe it's good to put it in a sub-package like you suggested, maybe something like solrutils.

@takanuva15
Copy link
Contributor Author

Hmm... yes we can definitely do that. My only concern is that it could make this package more confusing since developers will now have to look between 2 packages to figure out whether the function they want is in solr or solrutils.

For example, in the Golang standard library, it seems like they usually just group all related stuff in a single package. For example, strings.Builder is a dedicated struct for string building/appending (just like solr.NewQuery), but it's also in the same package as strings.Join, which is just a utility function for joining any array of strings together. Here, it looks like the Golang team intentionally did not make a separate package stringutils for strings.Join, even though it's technically separate from strings.Builder.

Along similar lines, it seems like it would match closer to the design of the Golang standard library to keep all the Solr stuff under a single package name solr.

@stevenferrer
Copy link
Owner

Hmm... yes we can definitely do that. My only concern is that it could make this package more confusing since developers will now have to look between 2 packages to figure out whether the function they want is in solr or solrutils.

For example, in the Golang standard library, it seems like they usually just group all related stuff in a single package. For example, strings.Builder is a dedicated struct for string building/appending (just like solr.NewQuery), but it's also in the same package as strings.Join, which is just a utility function for joining any array of strings together. Here, it looks like the Golang team intentionally did not make a separate package stringutils for strings.Join, even though it's technically separate from strings.Builder.

Along similar lines, it seems like it would match closer to the design of the Golang standard library to keep all the Solr stuff under a single package name solr.

I was worried that the solr namespace would be polluted by utility functions such as this, but if there isn't a lot of them, then I think this should be okay.

I'll merge and publish a new release later, thanks!

@stevenferrer stevenferrer merged commit ae44b04 into stevenferrer:main Feb 17, 2025
3 checks passed
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.

Add function for escaping special characters in query terms

2 participants