search

koopjs / winnow

Deprecated
Apache License 2.0
90 stars 18 forks source link

Bug/fix regex replacement bug #53

Closed haoliangyu closed 6 years ago

haoliangyu commented 6 years ago

bug

The pad() function in the src/where.js can not correctly work with clause with multiple >=/<=. For example, COMPTMENT >= 3010 AND FOREST >= 517.

reason

At the line 49 of src/where.js, space paddings are added to ALL operators, but at the line 51, it only removes the middle space for the FIRST >=/<= operator. It leaves additional spaces in other >=/<= operators and interrupts following steps.

fix

Do a global matching to remove all unnecessary spaces.

haoliangyu commented 6 years ago

Close this PR in favor of #54

jgravois commented 6 years ago

@haoliangyu usually its helpful to push additional commits onto the branch associated with a pull request that's already open after/during code review to keep the conversation all in one place, even if you rework your approach entirely.

haoliangyu commented 6 years ago

@jgravois But #53 and #54 are two competing/conflicting solutions to the same problem (though #54 is a more fundamental update). Shouldn't them be in separate branches and PRs?

jgravois commented 6 years ago

if they are truly in competition, then you're right. it would make sense to maintain both.

i took you closing this PR to mean that you prefer the approach in #54 and were withdrawing the approach here from consideration. if i misunderstood, please carry on.

haoliangyu commented 6 years ago

@jgravois No problem. I should have stated it clearer. @dmfenton and I think #54 is a better solution for the long term maintenance and we prefer it. I open this PR basically to provide a solution based on the current implementation in case #54 doesn't work well.

I am closing this PR again as #54 is merged.