-
Notifications
You must be signed in to change notification settings - Fork 12
refactor: use a repo-metadata to generate the readme #6
Conversation
|
||
# Google Cloud Firestore Sessions | ||
# [Google Cloud Firestore Session: Node.js Client](https://github.com/googleapis/nodejs-firestore-session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcoe this library doesn't fit the traditional mold, in that it's a connector sorta thing. The title is sorta goofy - any ideas how we can better support this in the partials template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, do you think we should just let you override the title; make it optional in the partials?
Any other thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think being able to override the title would be lovely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other parts of the template that feel clunky to you with this type of library?
What came to my mind as well, was does this make sense to open in Cloud Shell? I suppose someone could run the quick-start server here and play with it?
Actually.... it's great. Just the title would help a lot :) |
@@ -36,7 +36,7 @@ module.exports = { | |||
includePattern: '\\.js$' | |||
}, | |||
templates: { | |||
copyright: 'Copyright 2019 Google, LLC.', | |||
copyright: 'Copyright 2018 Google, LLC.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the year?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file gets copied in with the other templates, so it would be kinda hard to change this. I think the copyright is supposed to be the year the code was written, so this is probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
```javascript | ||
const Firestore = require('@google-cloud/firestore'); | ||
const {Firestore} = require('@google-cloud/firestore'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When did Firestore start to return an object with the Firestore
field? I used it last night and it's `const Firestore = required('@google-cloud/firestore');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact... it supports both! The code here is copied in from the quickstart
, which does have test coverage. So this should be fine :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think const {Firestore}
is wrong and some of the copyright years are off.
Thank you 😀 |
This uses a
.repo-metadata.json
to generate the README, and makes a few other minor changes to support it. Fixes #5.