Evergreen

Bug #1152863
Comment #26

Comment 26 for bug 1152863

Revision history for this message

Mike Rylander (mrylander) wrote on 2013-10-10:

#26

Scott and Fredrick, thanks for following up on this. I appreciate you taking the time to lay out your thoughts. I'll copy and paste from above for clarity.

fparks commented above:

> In short, we didn’t want to change how something works for everyone for the request coming from a subset of the community (MassLNC). If it turns out however that the majority of the community would want to use language “and/or/not” vs “&&/||/-“ then the change to query parser would make the most sense.

I understand that desire, and even empathize with it, but the rest of the community is then burdened with not breaking MassLNC's requested feature, especially because MassLNC always tries to work within the community -- or, MassLNC is burdened with unbreaking it when others adjust adjacent code. It is, perhaps, a difference of style, but I would start from the other end and say, "this looks generally useful, and there is code to support this already in an integrated way, so let's start from the position of a fully-baked feature."

But, excepting everything I just said, it seem clear that other users certainly would like something that looks like this feature, as evidenced by the activity on this bug -- there are comments receptive to the concept of the feature appearing with 10 days of the initial post.. IMO, we passed the tipping point pretty quickly.

> I would suggest I18n be implemented in the front end of any project. The underlying business logic should never depend on what language is being use. This is copying how .dtd is currently being used in other areas of Evergreen.

All other locations that use DTDs in Evergreen do so through an XML processor. I don't think there is any other place in the code where the contents of a DTD are being used to drive logic. You're generating regexp's based on DTD content, and while the case of English seems safe, others may very well not be. IMO, and accepting that perhaps we're misunderstanding each other, it isn't a correct statement to say that your use of DTDs is the same as other uses in Evergreen.

To sum up my response to fparks, I'm also in favor of the functional request, but my concerns remain about the implementation.

Later, smyers wrote:

> I can see a couple of problems with going with the query parser implementation.
>
> 1. The only search that needs a changed query parser is the boolean search which would mean needing to pass a flag all the way from the search page to the query parser which would be fragile for people unaware of it.

I see that as a straw-man, as it assumes that is either novel or the only available mechanism. First, there are many fields passed along with a search, and second, the search can be made self-configuring -- I can imagine a query modifier (say, #human_boolean) turning this functionality on for a specific scope within the search.

> 2. The logic on when to change the "and" to && is different for boolean search to handle the following correctly.
> "Lilo and Stitch"
> In the boolean search any qutoed search item gets searched on exactly as entered by the user. There is no functionality that matches that in query parser currently as I know it.
>

That's another straw-man, because quoted searches work today, and of course would need continue to work. QueryParser knows when it is looking at a phrase, and can handle that fine.

> 3. The current implementation works.
>

This is the argument that strikes home for me the most, actually.

As a project and a community, we have generally had that attitude that "working code wins," and that has usually been to our benefit. However, when the codebase was smaller and most folks working on it understood all the moving parts, that was a proposition that carried less risk than today. My concern, and I think this aligns with the general consensus of the broader community, is that we have grown past that stage, and the risk of adding features without considering their total impact is too high to allow "working code wins" to always rule the day.

It's also a philosophical argument, though. I still won't stop any other committer that believes this is ready for master to merge it, but my concerns still remain.

> If we were to use the query parser to handle the boolean search these items would necessitate making a some changes to a part of the code that has been thoroughly tested.
>

Sure, but that part of the code is also intended to address exactly this functional area. It's the tool designed for this job.

> With that in mind if moving the logic into query parser will get this into master so MassLNC can use it we would be happy to do so.

Obviously, I'm in favor of this. If your team decides to go this route, I would recommend as much public discussion (and question asking, to clarify the intent of existing code wherever you're unsure) as possible before pushing for commit-ready code. Dan's correct, as well. There are several of us that have had our hands inside QueryParser (both the main module and the Evergreen driver for it) and we'd be happy to help clarify anything that is opaque.

Scott and Fredrick, thanks for following up on this.  I appreciate you taking the time to lay out your thoughts.  I'll copy and paste from above for clarity.

fparks commented above:

I understand that desire, and even empathize with it, but the rest of the community is then burdened with not breaking MassLNC's requested feature, especially because MassLNC always tries to work within the community -- or, MassLNC is burdened with unbreaking it when others adjust adjacent code.  It is, perhaps, a difference of style, but I would start from the other end and say, "this looks generally useful, and there is code to support this already in an integrated way, so let's start from the position of a fully-baked feature."

All other locations that use DTDs in Evergreen do so through an XML processor.  I don't think there is any other place in the code where the contents of a DTD are being used to drive logic.  You're generating regexp's based on DTD content, and while the case of English seems safe, others may very well not be.  IMO, and accepting that perhaps we're misunderstanding each other, it isn't a correct statement to say that your use of DTDs is the same as other uses in Evergreen.

To sum up my response to fparks, I'm also in favor of the functional request, but my concerns remain about the implementation.

Later, smyers wrote:

> I can see a couple of problems with going with the query parser implementation.
> 
> 1. The only search that needs a changed query parser is the boolean search which would mean needing to pass a flag all the way from the search page to the query parser which would be fragile for people unaware of it.

I see that as a straw-man, as it assumes that is either novel or the only available mechanism.  First, there are many fields passed along with a search, and second, the search can be made self-configuring -- I can imagine a query modifier (say, #human_boolean) turning this functionality on for a specific scope within the search.

> 2. The logic on when to change the "and" to && is different for boolean search to handle the following correctly.
>      "Lilo and Stitch"
>    In the boolean search any qutoed search item gets searched on exactly as entered by the user. There is no functionality that matches that in query parser currently as I know it.
>

That's another straw-man, because quoted searches work today, and of course would need continue to work.  QueryParser knows when it is looking at a phrase, and can handle that fine.

> 3. The current implementation works.
>

This is the argument that strikes home for me the most, actually.

As a project and a community, we have generally had that attitude that "working code wins," and that has usually been to our benefit.  However, when the codebase was smaller and most folks working on it understood all the moving parts, that was a proposition that carried less risk than today.  My concern, and I think this aligns with the general consensus of the broader community, is that we have grown past that stage, and the risk of adding features without considering their total impact is too high to allow "working code wins" to always rule the day.

It's also a philosophical argument, though.  I still won't stop any other committer that believes this is ready for master to merge it, but my concerns still remain.

> If we were to use the query parser to handle the boolean search these items would necessitate making a some changes to a part of the code that has been thoroughly tested.
>

Sure, but that part of the code is also intended to address exactly this functional area.  It's the tool designed for this job.

> With that in mind if moving the logic into query parser will get this into master so MassLNC can use it we would be happy to do so.

Obviously, I'm in favor of this.  If your team decides to go this route, I would recommend as much public discussion (and question asking, to clarify the intent of existing code wherever you're unsure) as possible before pushing for commit-ready code.  Dan's correct, as well. There are several of us that have had our hands inside QueryParser (both the main module and the Evergreen driver for it) and we'd be happy to help clarify anything that is opaque.