-
Notifications
You must be signed in to change notification settings - Fork 37
chore: adds @license JSDoc tag to license comments #645
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #645 +/- ##
=======================================
Coverage 98.72% 98.73%
=======================================
Files 7 7
Lines 2046 2052 +6
Branches 173 173
=======================================
+ Hits 2020 2026 +6
Misses 26 26
Continue to review full report at Codecov.
|
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
/** |
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.
Some time ago we tried to standardize on //
style comments in our modules instead of /** **/
for license headers. To use @license
, do you need to use multiline comments? Or would single line comments continue to work?
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.
Hmm just tool a look at the import script in g3 and it appears to only look for @license
in multiline comments.
If this is a deal breaker I can just update the license manually in g3 for now and add a local modification note. I can also file a bug to see about supporting single line comments in the importer.
Let me know which you prefer, but yeah at least for now, this style of license comment doesn't play well with the g3 importer :/
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.
So this could be possibly be fine. We have ~100 node.js GitHub repositories, and we like to keep things consistent. If you need this added everywhere, it may be faster us to script a broad change that fixes the header in every file all at once 😬
@alexander-fenster @bcoe this could be an adventure
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.
A starting point might be updating the typescript generator and the templates here which are also populated in repositories.
@nathanmosher if this change provides enough value to be worth this work, I would start by making changes in these places.
Are there specific libraries in g3 that you'd like the headers for?
@nathanmosher if we are interested in doing this widely, let's add the new headers to the generator 👍 |
Hi, this change just formats the license blocks to play a bit better when importing into Google3, similar to a change I've made in another Google NodeJS library: firebase/firebase-admin-node#1102
Let me know if this looks ok or if you need anymore context.