Closed ace-n closed 2 years ago
So far LGTM, but there are still quite a few sections missing Node.JS samples. Is this intentional?
@kurtisvg I was omitting Node samples for the non-Node specific stuff (to reduce maintenance load + visual clutter).
If you'd rather I change that, I'm happy to do so though!
@ace-n I think it make's sense to have a different sample for each language, even if there aren't any major derivations.
Also, maybe tangentially, I noticed that "Node.js" tabs look like they are broken -when I click on one tab it doesn't switch all the Tabs, but it does for "Java". I switched the tabs to "Node" and the tab switching works, but the linting is lost. I switched to "Javascript" and both worked?
N.B: this is running into merge conflicts due (mostly) to whitespaces. 😬
Once this PR is approved, I'll close it and create a clean version elsewhere (that we can rubber-stamp and merge).
@sofisl and I chatted about the DLP
/dlp
issue. We're reluctant to rename DlpServiceClient
(because it's export
ed under that name), but the initialized client object could be named something like dlpClientInstance
(instance
being the key word here).
@grayside does this address your concerns?
@sofisl over to you for one final (double-check-y) LGTM. 🙂
Please merge for me once you've approved - thanks!
~DO NOT MERGE until everyone's in sync on client library initialization (
DLP
/dlp
shenanigans).~~Note: there are lots of merge conflicts due to whitespace changes. Once we've approved this PR, I can fix those locally and force-push the results to the
add-nodejs
branch.~