Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deprecated StringProcessor #47

Merged
merged 1 commit into from
Oct 30, 2018
Merged

Deprecated StringProcessor #47

merged 1 commit into from
Oct 30, 2018

Conversation

burdoto
Copy link
Contributor

@burdoto burdoto commented Oct 30, 2018

Deprecated StringProcessor and replaced all occurrences with ToStringFunction<String>, which was implemented with 1.2.0, because it is basically a more ""primitive"" implementation and theoretically a duplicate.
StringProcessor is now an abstract class, because the JDK 1.7 does not allow default methods in interfaces, which would have been required for backwards compatibility. That way, StringProcessor can implement ToStringFunction<String> and override apply(T) to overload process(String).

NoProcess got deprecated and replaced with ToStringFunction.DEFAULT
DefaultStringProcessor got deprecated so that people switch to the properly named DefaultStringFunction

Deprecated StringProcessor and replaced all occurrences with ToStringFunction<String>, which was implemented with 1.2.0
StringProcessor is now an abstract class, because the JDK 1.7 does not allow default methods in interfaces, which would have been required for backwards compatibility.

Also, NoProcess got deprecated and replaced with ToStringFunction.DEFAULT
@burdoto
Copy link
Contributor Author

burdoto commented Oct 30, 2018

Note that I am well aware of the junk-ness of making the StringProcessor class now an abstract class, but to me it seemed the easiest solution to keep the most backwards compatibility, because this way, simple calls like the following still work fine :

StringProcessor processor = new StringProcessor() {
[...]

After all, it is deprecated and should be replaced in the first place, but this will work in any case that does not use an implementing class.

@xdrop
Copy link
Owner

xdrop commented Oct 30, 2018

Nice one! Thanks for this.

I also realised yesterdays changes (#46) break all extractAll calls (as List<ExtractedResult<String>> is not assignable to List<ExtractedResult>)

List<ExtractedResult> res = FuzzySearch.extractAll("hey", new ArrayList<String>(), 20); // error

We could let the old ones be untyped (with a rather ugly cast):

public static List<ExtractedResult> extractAll(String query, Collection<String> choices, int cutoff) {

     Extractor extractor = new Extractor(cutoff);

     return (List<ExtractedResult>)(List<?>)extractor.extractWithoutOrder(query, choices, ToStringFunction.DEFAULT, new WeightedRatio());
}

Or preferably introduce another type eg. (TypedExtractedResult<T> or TypedResult<T> or a better name if you can come up with one ) that is typed and leave ExtractedResult as it was.

We could then merge the types when a new major version gets released, but until then let's not break backward compatibility.

return this;
}

public BasicAlgorithm noProcessor(){
this.stringProcessor = new NoProcess();
this.stringFunction = ToStringFunction.DEFAULT;
Copy link
Owner

@xdrop xdrop Oct 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets replaced by DEFAULT which I believe is not the same as NoProcess, the basic ratios should not have any processing done to them by default, so we'd still need to have something that represents an identity string function input => input for this usecase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoProcess just returned the unhandled input String, which is exactly what DEFAULT does, so I fail to see a difference. 🤔 https://github.com/xdrop/fuzzywuzzy/pull/47/files#diff-a5cadeadf032c3e499ee1134add85c93L9

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you got confused between ToStringFunction.DEFAULT and DefaultStringFunction?
The naming is odd on those ones.
What could be a better name for the final DEFAULT variable?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh my bad yeah got confused between those two. Maybe we should call it ToStringFunction.NO_PROCESS or ToStringFunction.IDENTITY?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would go for "NO_PROCESS". will be contained in my next PR

@burdoto
Copy link
Contributor Author

burdoto commented Oct 30, 2018

It seems like you have forgot that every non-referring extract method returns a List<ExtractedResult<String>> instead of a List<ExtractedResult>; where the string is both the "string" and the "referent" value of each ExtractedResult

@burdoto
Copy link
Contributor Author

burdoto commented Oct 30, 2018

Oh welp, I have just realized the actual problem.

Yea, sure. I agree on adding a different ExtractedResult item, what should its name be? i suggest ExtractedReferentResult

Also, maybe you wanna make a branch "2.0" where we can form a new major release already

@xdrop
Copy link
Owner

xdrop commented Oct 30, 2018

I've made a branch 'develop' for the next major. NO_PROCESS sounds good. Not quite sure on ExtractedReferentResult since I've had to google to see what referent means 😅

You can push that for now though and I'll rename it later if I can think of anything better.

@burdoto
Copy link
Contributor Author

burdoto commented Oct 30, 2018

i have changed the name to BoundExtractedResult

@burdoto
Copy link
Contributor Author

burdoto commented Oct 30, 2018

also im having the issue that i failed to remove the Deprecation commit before branching these changes.
If you wanna merge this, i would highly appreciate if you could do that right now, so that we wont have any conflicts

@xdrop
Copy link
Owner

xdrop commented Oct 30, 2018

Nice, looking good - gonna keep this name.

@xdrop xdrop merged commit c63c904 into xdrop:master Oct 30, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants