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

Consolidated Requests #28

Merged
merged 5 commits into from
May 30, 2018
Merged

Conversation

LaughDonor
Copy link
Collaborator

@LaughDonor LaughDonor commented May 23, 2018

Consolidated all the requests to a centralized object. #22
Cleaned up some logic and added comments.

@LaughDonor
Copy link
Collaborator Author

Issues mentioned in #26 have been fixed.

  • .getDocument(Id)s have been cleaned up
  • .query is more stable. Will accept any type of path, and resolve collection IDs automatically

@LaughDonor
Copy link
Collaborator Author

I've deprecated the createDocumentWithId function since createDocument can create an ID if a document name isn't given in the path. Goes with the consolidation. Let me know your thoughts @grahamearley.

@grahamearley
Copy link
Owner

@LaughDonor Awesome. Haven't had a change to do any testing with this one yet, but after a quick read-through it looks like another great PR. I'll get back to you with some more notes after I test it out.

I like the idea of consolidating the document creation methods — I say let's delete createDocumentWithId entirely. Would you mind updating the README in this PR then, to reflect that that method isn't there anymore?

Again — looks great. I'll get back to you soon!

README.md Outdated
See other library methods and details [in the wiki](https://github.com/grahamearley/FirestoreGoogleAppsScript/wiki/Firestore-Method-Documentation).

### Breaking Changes
* v16: `createDocumentWithId(documentId, path, fields)` will be **removed** in the next update.
Copy link
Owner

Choose a reason for hiding this comment

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

By this do you mean that method will be removed in v17?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why not? Unless you would like to just remove it immediately. I'm good either way.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I'm leaning toward just removing it in this PR. I don't think this library is used at a large enough scale that this breaking change would wreak any havoc, so I say let's just do it.

Good idea to call it out in the readme!

@LaughDonor LaughDonor force-pushed the api-calls branch 2 times, most recently from a71d67b to c1343ff Compare May 29, 2018 20:41
README.md Outdated
firestore.createDocument("FirstCollection", data)
```

Alternatively, we can create the document in the `FirstCollection` collection called called `FirstDocument`:
Copy link
Owner

Choose a reason for hiding this comment

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

You have two "called"s in a row here :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😯

@LaughDonor
Copy link
Collaborator Author

All cleaned up. Thanks for the checks!

Copy link
Owner

@grahamearley grahamearley left a comment

Choose a reason for hiding this comment

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

Other than my one comment, this is ready to go! I'll approve once you address my comment.

*/
this.getDocumentIds = function (pathToCollection) {
return getDocumentIds_(pathToCollection, authToken, projectId)
this.updateDocument = function (path, fields) {
Copy link
Owner

Choose a reason for hiding this comment

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

updateDocument appears twice, and seems to be identical — copy paste error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I was in preparation of the next PR, and forgot to undo. All fixed up @grahamearley

@grahamearley
Copy link
Owner

grahamearley commented May 29, 2018

@LaughDonor Do you have any ideas on how to update our method documentation in this repo's wiki more automatically? I'd rather not have to keep manually typing it up when there are changes, but I'm not well-versed in Javascript, so I'm unaware of any nice auto-markdown or documentation generation tools.

Any thoughts?

We'll have to get the wiki updated when this gets merged in and I release a new version.

EDIT: Found a solution! https://github.com/documentationjs/documentation. Just what I was looking for.

Sunny Patel added 2 commits May 30, 2018 11:40
…ection ID.

Deprecated createDocumentWithId. Can utilize createDocument with any path.
Clean up getDocuments* functions.
Updated README with more details.
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