Misc. manual formatting #623

irace-stripe · 2019-05-09T20:40:45Z

Some misc. manual formatting to get us closer to the code style that we’re striving for:

No chained method calls on require statements
Paths start with /
Line break between path and includeBasic entries in object literals
Alphabetical method ordering
Remove misc. comments

ob-stripe · 2019-05-09T20:44:55Z

lib/resources/CreditNotes.js

  includeBasic: ['create', 'list', 'retrieve', 'update'],

  voidCreditNote: stripeMethod({
    method: 'POST',
-    path: '{id}/void',
+    path: '/{id}/void',


Is that correct? Other paths do not have a leading /.

@ob-stripe Some do, some don’t. I’m assuming they should all be consistent at the very least and unless I’m misremembering, @rattrayalex-stripe suggested leading /

I did. I'm pretty sure we have something that normalizes this. just in case, @irace-stripe can you check that there is test coverage for credit notes?

There seems to be, though I’m not 100% sure if these tests cover the difference between leading / and no leading /? https://github.com/stripe/stripe-node/blob/master/test/resources/CreditNotes.spec.js

https://github.com/stripe/stripe-node/blob/master/test/resources/CreditNotes.spec.js#L62-L65 seems to give us coverage – if the leading slash was problematic, it would fail since the request would include a //.

rattrayalex-stripe · 2019-05-09T20:53:40Z

lib/resources/LoginLinks.js

@@ -3,6 +3,7 @@
 const StripeResource = require('../StripeResource');

 module.exports = StripeResource.extend({
-  path: 'accounts/{accountId}/login_links',
+  path: 'accounts/{account}/login_links',


this should be accountId, no?

@rattrayalex-stripe Not as per OpenAPI, but we can override if necessary?

rattrayalex-stripe · 2019-05-09T20:54:20Z

lib/resources/TransferReversals.js

 module.exports = StripeResource.extend({
-  path: 'transfers/{transferId}/reversals',
+  path: 'transfers/{id}/reversals',


this should be transferId right?

@rattrayalex-stripe Not as per OpenAPI, but we can override if necessary?

ahhh interesting

I think passing openapi's view of the world through is fine, carry on

rattrayalex-stripe · 2019-05-09T20:54:58Z

few nits on nested resource url param names, otherwise this looks magnifico!

r? @irace-stripe

rattrayalex-stripe · 2019-05-09T21:04:26Z

LGTM

pending build failure

irace-stripe · 2019-05-09T21:43:48Z

@rattrayalex-stripe I had to make a bunch of unit test changes in response to having changed the URL parameter names: d8067a8#diff-b0d90832c1cb39522f0108600be90655. This scares me a little bit since how would users of this library going to know that they would have to do this?

There’s still one failing test that I’m unsure how to fix, unfortunately.

rattrayalex-stripe · 2019-05-09T21:48:31Z

ahhhh shoot. We weren't sure if the url params could be set with a hash; I was under the impression they couldn't be.

This definitely implies that the url params are part of our public API, and need to be preserved, which is super sad since we're so inconsistent about it today.

Actually, on that note, should we just make a backwards-incompatible change as part of Node 7, @ob-stripe ?

Either way, I would say that we should not use openapi's url param names (which are not consistent) and should instead rename the url params with a consistent format.

For something not named, {id} would be good, but I think probably {resourceId} is best given that users can pass this as a hash.

Thoughts?

rattrayalex-stripe · 2019-05-09T21:49:29Z

(stepping back, I think let's pull the url params into a separate PR, and merge everything else here – what do you say, @irace-stripe ?)

irace-stripe · 2019-05-09T21:55:33Z

@rattrayalex-stripe Yes, that sounds right to me. Will work on that.

Question: does that also need we need to roll back changes like https://github.com/stripe/stripe-node/pull/606/files#diff-82d7692da4c2b29e9c7c59e40e28531bL16?

irace-stripe · 2019-05-09T22:02:42Z

lib/resources/SubscriptionScheduleRevisions.js

- * E.g. scheduleObject.revisions.retrieve(revisionId)
- * (As opposed to the also-supported stripe.subscriptionSchedules.retrieveRevision(scheduleId,
- * revisionId))
- */
 module.exports = StripeResource.extend({
  path: 'subscription_schedules/{scheduleId}/revisions',


@rattrayalex-stripe Is it weird that we’re standardizing on leading / for methods paths but not for resource paths?

IMO we should have leading slashes on both, yes

…de into irace-manual-formatting

rattrayalex-stripe · 2019-05-09T22:30:45Z

Question: does that also need we need to roll back changes like https://github.com/stripe/stripe-node/pull/606/files#diff-82d7692da4c2b29e9c7c59e40e28531bL16?

If that's the direction we want to go, yes.

irace-stripe · 2019-05-09T22:32:31Z

If that's the direction we want to go, yes.

Noted. Those method paths might be far more painful to override, unfortunately

rattrayalex-stripe · 2019-05-09T22:33:26Z

Yeah, I'd rather not override, just want to decide whether, for non-nested resources, we want /foo/{id} or /foo/{fooId}

irace-stripe · 2019-05-09T22:34:38Z

Yeah, I'd rather not override, just want to decide whether, for non-nested resources, we want /foo/{id} or /foo/{fooId}

Unifying on either one would mean breaking public API for the other 😐

rattrayalex-stripe · 2019-05-09T22:45:25Z

Yep! Now is the time to do it.

* Misc. manual formatting * Fix some unit tests * Roll back path argument name changes * Misc. manual formatting

* Update version to 7 and drop support for Node 4 and Node 5, and Node 7 * Format using Prettier tmp * Alphabetize basic methods * Use 'id' for all single identifier positional arguments * Fix typo * Modernize ES5 to ES6 with lebab (#607) * Add lebab and a script to run it * lebab transform: arrow * lebab transform: arg-rest * lebab transform: arg-spread * lebab transform: obj-method * lebab transform: obj-shorthand * lebab transform: let * lebab transform: template * lebab transform: default-param * lebab transform: destruct-param * lebab transform: includes * Revert "Add lebab and a script to run it" This reverts commit 70fd492. * Revert "lebab transform: destruct-param" because its changes didn't seem good. This reverts commit b56f52d. * Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible. This reverts commit 7eba992. * Unrelated: mark 8.1 as minimum 8-series version * Add mocha-only script * Use arrows in more places * Loosen some eslint rules I don't love * Remove deprecated methods * Add VSCode and EditorConfig files * Bump dependencies to latest versions * Remove legacy parameter support in invoices.retrieveUpcoming() * Misc. manual formatting (#623) * Misc. manual formatting * Fix some unit tests * Roll back path argument name changes * Misc. manual formatting * Remove "curried" nested resources and manually specified urlParams (#625) * Drop support for optional url params * Delete nested resource files * Remove urlData * Extract urlParams from path instead of manual definition Verified this is no different with: ```js const urlParams = utils.extractUrlParams(spec.path || ''); if ( !(spec.urlParams || []).every((x, i) => urlParams[i] === x) || (spec.urlParams || []).length !== urlParams.length ) { throw Error( 'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams) ); } ``` inside StripeMethod * Remove manually specified urlParams * Add a deprecation error message * Revert "Delete nested resource files" This reverts commit d88a3e7. * Fix nested resources for non-curried urlParams and update tests to demonstrate their use * Refactor makeRequest * Revert "Revert "Delete nested resource files"" This reverts commit e5eccb8. * Extract resources file (to aid with code generation) (#626) * Extract separate resources file (to aid with code generation) * Remove resources that were removed in #625 https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85 * Update CHANGELOG for 7.0.0 release

Misc. manual formatting

8a0c948

stripe-ci assigned rattrayalex-stripe and ob-stripe May 9, 2019

ob-stripe reviewed May 9, 2019

View reviewed changes

rattrayalex-stripe reviewed May 9, 2019

View reviewed changes

Fix some unit tests

d8067a8

Roll back path argument name changes

a47767d

irace-stripe commented May 9, 2019

View reviewed changes

irace-stripe added 2 commits May 9, 2019 18:03

Misc. manual formatting

9659c1a

Merge branch 'irace-manual-formatting' of github.com:stripe/stripe-no…

b64ea0d

…de into irace-manual-formatting

rattrayalex-stripe approved these changes May 9, 2019

View reviewed changes

stripe-ci added the approved label May 9, 2019

stripe-ci assigned irace-stripe and unassigned ob-stripe and rattrayalex-stripe May 9, 2019

irace-stripe merged commit a1be521 into v7.0.0 May 9, 2019

irace-stripe deleted the irace-manual-formatting branch May 9, 2019 22:33

rattrayalex-stripe mentioned this pull request May 9, 2019

stripe-node v7.0.0 #606

Merged

13 tasks

rattrayalex-stripe pushed a commit that referenced this pull request May 14, 2019

Misc. manual formatting (#623)

ad3ad5b

* Misc. manual formatting * Fix some unit tests * Roll back path argument name changes * Misc. manual formatting

rattrayalex-stripe pushed a commit that referenced this pull request May 14, 2019

Misc. manual formatting (#623)

3ca459a

* Misc. manual formatting * Fix some unit tests * Roll back path argument name changes * Misc. manual formatting

rattrayalex-stripe pushed a commit that referenced this pull request May 14, 2019

Misc. manual formatting (#623)

f9592c9

* Misc. manual formatting * Fix some unit tests * Roll back path argument name changes * Misc. manual formatting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc. manual formatting #623

Misc. manual formatting #623

irace-stripe commented May 9, 2019

ob-stripe May 9, 2019

irace-stripe May 9, 2019

rattrayalex-stripe May 9, 2019

irace-stripe May 9, 2019

rattrayalex-stripe May 9, 2019

rattrayalex-stripe May 9, 2019

irace-stripe May 9, 2019

rattrayalex-stripe May 9, 2019

irace-stripe May 9, 2019

rattrayalex-stripe May 9, 2019

rattrayalex-stripe May 9, 2019

rattrayalex-stripe commented May 9, 2019 •

edited

Loading

rattrayalex-stripe commented May 9, 2019

irace-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019

irace-stripe commented May 9, 2019

irace-stripe May 9, 2019

rattrayalex-stripe May 9, 2019

rattrayalex-stripe commented May 9, 2019

irace-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019

irace-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019

Misc. manual formatting #623

Misc. manual formatting #623

Conversation

irace-stripe commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex-stripe commented May 9, 2019 • edited Loading

rattrayalex-stripe commented May 9, 2019

irace-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019

irace-stripe commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex-stripe commented May 9, 2019

irace-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019

irace-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019

rattrayalex-stripe commented May 9, 2019 •

edited

Loading