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

Suggested fix(inequality filter) #80

Merged
merged 5 commits into from Jan 21, 2019

Conversation

spoxies
Copy link
Contributor

@spoxies spoxies commented Jan 18, 2019

Refers to #78

src/GeoQuery.ts Outdated
@@ -139,6 +140,9 @@ export class GeoQuery {
opStr: GeoFirestoreTypes.WhereFilterOp,
value: any
): GeoQuery {
// True if inequality filter is used
this._ineq = (opStr !== '==' && opStr !== 'array-contains') ? true : this._ineq;
Copy link
Owner

Choose a reason for hiding this comment

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

Can we yank this, I'd rather go straight back to using return this._query.orderBy('g').startAt(query[0]).endAt(query[1]) as GeoFirestoreTypes.web.Query;

So any inequality checks can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

The argument was that by using where() instead of startAt, the "orderBy query modifiers" could be used by devs. Therefore I wanted to respect that change. However I agree and with the " inequality checks" the behaviour becomes inconsistent.

@spoxies
Copy link
Contributor Author

spoxies commented Jan 21, 2019

It became apparent that this PR probably does not have any added value 😬 see #78

@MichaelSolati MichaelSolati merged commit 52c268c into MichaelSolati:dev Jan 21, 2019
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.

None yet

2 participants