-
Notifications
You must be signed in to change notification settings - Fork 304
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
OData lambda style filter query #3155
Conversation
This solved the issue where if you reused the same variable name in your long queries it broke
This reverts commit fc6fd40.
down to one warning which I can't resolve
# Conflicts: # package-lock.json # packages/sp/spqueryable.ts
Latest commit added "auto typing" for things like lists, webs, and so on, where we "know the type" I.e await sp.web.webinfos.filter(w => w.text("Title").equal('MyWebTitle'))(); Without having to specify that I haven't tested this in every possible combination, but it looks to be working without breaking the list item stuff, should greatly improve the usability 😊 |
Thanks! Will be doing some testing on this. |
@bcameron1231 Let me know if you have any issues, I have some initial work on the first draft of the docs, do you want them here, or in a separate PR? |
Let's get those in the same commit. It will also help me test correctly. 😁 Excited to see this. Can you also try and squash your commits? |
We like "method 1", seems simpler Drop "Field" from all the names so Lookup() vs LookupField(). switch to camel case so Lookup becomes lookup() could simplify the names, equal instead of EqualsTo and notEqual vs NotEqualTo Swap and/or for All/Some, just seems clearer to someone showing up with no knowledge of the filter methods pass in multiple objects you can use ...[] to gather them into an array within the method. this should be able to support all the filter cases, another reason to drop the "Field" name from the builder functions. Like for a site if you give it ISiteInfo you could filter on Text("Title").EqualsTo("blah");
828fd46
to
76dca87
Compare
So this might just be my git skills that are lacking - I can't seem to squash the old commits without ending up with like 300 merge confilts that need manual processing, but I at least got the newest ones squashed into two |
Thank you. I've done some testing, it looks to be working. However it looks likes the plurals may be gone? i.e, .equal() vs .equals(). We'd like the plurals back 👍 Here is what we plan to do once you make those changes. Given this has backwards compatibility, we're going to get this merged in and try to push it out in November. Our goal is to promote this new functionality in the next Month's call as a beta functionality. Our hope is that we can get others to start using it and gather some feedback on the implementation. Maybe we could get you set up to do a Demo on a call at some point to explain how it works. |
I think the rename was just down to misunderstanding one of the last feedback items 😊 - changed it up for Looking forward to getting some real world feedback once it hits beta! |
🚀🚀🚀🚀🚀🚀 |
Are the lint rules different on the runs here and the settings in the project? 😅 - I only had one warning locally, which I couldn't get rid of 👀 |
Category
Related Issues
Based off the discussion over on #2781
What's in this Pull Request?
This PR is not completely done, but I needed a place to track the final changes (and docs) based off the input in #2781
.eq(...)
alongside the others, would like opinionsI'll happily start updating the docs once we have a "somewhat sign-off" on the above been done to an acceptable state 😊