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

fix: move score property into ParseQueryScorable #319

Merged
merged 4 commits into from
Jan 17, 2022

Conversation

cbaker6
Copy link
Contributor

@cbaker6 cbaker6 commented Jan 16, 2022

New Pull Request Checklist

Issue Description

Swift SDK 3.0.0 made all ParseObjects's have to have the property var score: Double? though this property is only needed when using the matchesText QueryConstraint.

Related issue: #n/a

Approach

Move the var score: Double? to a protocol named ParseQueryScorable. When developers want to sort by score using a matchesText QueryConstraint, they just conform their ParseObject's to ParseQueryScorable. Looks like below:

Developers who are not using the score property, but have upgraded can do either of the following:

  1. Nothing, if you added the score property already or didn't add it, this fix won't break your code
  2. If you don't plan to use the score property, you can remove it from your ParseObject's as it won't break your code

TODOs before merging

  • Modify tests
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Jan 16, 2022

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jan 16, 2022

@pmmlo can you let me know what you think about this?

@pmmlo
Copy link
Contributor

pmmlo commented Jan 16, 2022

You are a rockstar! Sorry to make more work. I honestly liked it the way you had it.

But, if we’re embracing protocols, do you think it would make sense to move
public func sortByTextScore() -> Query<T> {} from Query.swift to ParseQueryScorable.swift to enforce using the var score together with sortByTextScore()?

I’m on mobile but nothing I could see other than that.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jan 16, 2022

But, if we’re embracing protocols, do you think it would make sense to move
public func sortByTextScore() -> Query {} from Query.swift to ParseQueryScorable.swift to enforce using the var score together with sortByTextScore()?

Not sure how to make this happen since sortByTextScore modifies the query and not the ParseObject. Do you have an idea on how to cary this out?

I think it’s okay if sortByTextScore remains in query as essentially some can sort and never look at score.

@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #319 (146285e) into main (08882d0) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #319      +/-   ##
==========================================
- Coverage   84.63%   84.61%   -0.02%     
==========================================
  Files         114      114              
  Lines       11850    11850              
==========================================
- Hits        10029    10027       -2     
- Misses       1821     1823       +2     
Impacted Files Coverage Δ
...eSwift/InternalObjects/BaseParseInstallation.swift 36.36% <ø> (ø)
Sources/ParseSwift/Objects/ParseObject.swift 81.46% <ø> (ø)
...rces/ParseSwift/Protocols/ParseObjectMutable.swift 100.00% <ø> (ø)
Sources/ParseSwift/Types/Query.swift 92.28% <ø> (ø)
Sources/ParseSwift/Types/QueryConstraint.swift 95.71% <ø> (ø)
Sources/ParseSwift/Objects/ParseUser.swift 80.50% <0.00%> (-0.20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08882d0...146285e. Read the comment docs.

@cbaker6
Copy link
Contributor Author

cbaker6 commented Jan 17, 2022

Thinking about this more, I may be able to add:

public func sortByTextScore() -> Query<T> where T: ParseObject & ParseQueryScorable {

but this will cause a breaking change to anyone who began using sortByTextScore() in 3.0.0. So, it's probably best to leave this method untouched for now.

@cbaker6 cbaker6 merged commit 3c3a6e8 into parse-community:main Jan 17, 2022
@cbaker6 cbaker6 deleted the scoreProtocol branch January 17, 2022 19:05
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