refactor: Miscellaneous changes for consistency #292

ltfschoen · 2018-10-14T23:25:25Z

Split Promise Examples 02 and 03 into both ApiPromise and ApiRx versions
Update Promise Examples 05 and 07 to use Keyring Testpairs
Add Capitalised comments.
Add Node Interface comments
Fix typos
Fix incorrect variable name
Use Yarn instead of Npm in guides
Explain how ApiRx.create() is equivalent to new ApiRx().isReady
Fix example since should be connected instead of duplicate disconnect. Add missing on ready to example
Update Typedocs examples to match Promises examples
Rename Api to ApiPromise or ApiRx depending on what's imported
Rename provider to wsProvider for consistency
Add missing semicolons
Change WS and HTTP ports in tests to the ones we usually use
Remove unnecessary blank lines

… Node Interface comment

… comments. Use Yarn. Fix typos. Show equivalence

…ble name

…nnect. Add missing on ready

…pi to ApiPromise

…i to ApiRx. Add Node Interface comment

jacogr

Text cleanups work, the actual code changes tries to make things DRY and takes away learning. Here duplication is fine - examples for rx & promise need to be duplicated, so we have learning per API.

jacogr · 2018-10-15T05:56:37Z

docs/examples/promise/05_read_storage/index.js

-// Our address for Alice on the dev chain
-const Alice = '5GoKvZWG5ZPYL1WUovuHW3zJBWBP5eT8CbqjdRY4Q6iMaDtZ';
+// Create an instance of the keyring that includes test accounts
+const keyring = testingPairs();


Here we are trying to keep to an example with "real use", rather than using test data. Hence not relying on pre-populated. So keep to base-empty keyring

jacogr · 2018-10-15T05:57:50Z

docs/examples/promise/03_listen_to_balance_change/index.js

-// Import the Api
-const { ApiPromise } = require('@polkadot/api');
+const exampleApiPromise = require('./exampleApiPromise').default;
+const exampleApiRx = require('./exampleApiRx').default;


Not here. The examples need to be done for rx, but that means it needs to be duplicated - here we are trying to keep it simple for users of the specific type. (And other examples, keep as it was)

Ok, so the Rx examples should be in the examples/rx directory:

examples/promise/03_listen_to_balance_change/index.js

examples/rx/03_listen_to_balance_change/index.js

So at https://polkadot.js.org/api/, the menu would display:

EXAMPLES
ApiPromise
Simple connect
Listen to blocks
Listen to balance change

ApiRx
Simple connect
Listen to blocks
Listen to balance change

jacogr · 2018-10-15T05:59:09Z

docs/examples/promise/07_transfer_dots/index.js

-  // Create an instance of the keyring
-  const keyring = new Keyring();
+  // Create an instance of the keyring that includes test accounts
+  const keyring = testingPairs();


Just the normal keypair, no shortcuts :) We want to show use (actually here the "Alice add" is also not needed since it is there - hence stick with the barer bones, it is about showing how to use, not showing helpers)

jacogr

Comments added.

Overall this one seems to have taken on a life on it's own, i.e. changes all over which doesn't seem to be in any way related to the actual PR descriptions. Let's rather keep them short and to-the-point where we can.

jacogr · 2018-12-04T12:50:11Z

README.md

+
+## tests
+
+1. [Run a local Substrate chain node](https://github.com/paritytech/substrate#61-hacking-on-substrate)


jacogr · 2018-12-04T12:52:27Z

docs/examples/promise/07_transfer_dots/index.js

  const transfer = api.tx.balances.transfer(BOB_ADDR, 12345);

-  // Sign the transaction using our account
+  // Show unsigned transaction (alternate viewing forms are toU8a, toString, toJSON)
+  // Use Polkadot-JS utility to shorten displayed transaction value


Not sure what this line refers to? (We are not doing any magic here, just the base toHex())

jacogr · 2018-12-04T12:52:59Z

docs/examples/promise/07_transfer_dots/index.js

-  // Sign the transaction using our account
+  // Show unsigned transaction (alternate viewing forms are toU8a, toString, toJSON)
+  // Use Polkadot-JS utility to shorten displayed transaction value
+  console.log(`Unsigned transaction: `, transfer.toHex());


Use ' quotes

jacogr · 2018-12-04T12:53:31Z

docs/examples/promise/07_transfer_dots/index.js

  transfer.sign(alice, aliceNonce);

+  // Show signed transaction
+  console.log(`Signed transaction: `, transfer.toHex());


Use ' quotes

jacogr · 2018-12-04T13:03:10Z

docs/examples/rx/07_transfer_dots/index.js

@@ -0,0 +1,49 @@
+const { combineLatest } = require('rxjs');
+const {  first, switchMap } = require('rxjs/operators');


Errant space before first

jacogr · 2018-12-04T13:09:37Z

packages/rpc-provider/src/http/index.ts

@@ -26,11 +26,11 @@ const l = logger('api-http');
 * <BR>
 *
 * ```javascript
- * import Api from '@polkadot/api/promise';
+ * import ApiPromise from '@polkadot/api/promise';


jacogr · 2018-12-04T13:09:43Z

packages/rpc-provider/src/ws/index.ts

@@ -48,11 +48,11 @@ const l = logger('api-ws');
 * <BR>
 *
 * ```javascript
- * import Api from '@polkadot/api/promise';
+ * import ApiPromise from '@polkadot/api/promise';


jacogr · 2018-12-04T13:12:30Z

packages/rpc-provider/test/mockHttp.ts

@@ -4,7 +4,7 @@

 import nock from 'nock';

-const TEST_HTTP_URL = 'http://localhost:9944';
+const TEST_HTTP_URL = 'http://localhost:9933';


Should not overlap with the actual node endpoints. (So changing it is correct, changing it ot an overlapping port is not)

jacogr · 2018-12-04T13:12:39Z

packages/rpc-provider/test/mockWs.ts

@@ -4,7 +4,7 @@

 import { Server } from 'mock-socket';

-const TEST_WS_URL = 'ws://localhost:9955';
+const TEST_WS_URL = 'ws://localhost:9944';


Revert, no overlap.

jacogr · 2018-12-04T13:12:48Z

packages/rpc-rx/src/index.ts

@@ -30,8 +30,8 @@ type CachedMap = {
 * import RpcRx from '@polkadot/rpc-rx';
 * import WsProvider from '@polkadot/rpc-provider/ws';
 *
- * const provider = new WsProvider('http://127.0.0.1:9944');
- * const api = new RpcRx(provider);
+ * const wsProvider = new WsProvider('http://127.0.0.1:9944');


…d console.logs

…n docs/examples

… docs/examples

ltfschoen · 2018-12-04T19:00:27Z

@jacogr i've pushed commits that address the review comments

Changes made since review.

jacogr · 2018-12-08T08:04:49Z

docs/examples/rx/03_listen_to_balance_change/index.js

+    .subscribe((current) => {
+      previous = previous || current;
+
+      // FIXME - refactor like the ApiPromise where only the balance change console.log's


Not convinced we actually want FIXMEs in the examples at all. It should just be simple and working.

jacogr · 2018-12-08T08:09:11Z

packages/api/README.md

 ```

 Subscribing to blocks via Promise-based API -

 ```javascript
-import { ApiPromise } from '@polkadot/api';
+import Api from '@polkadot/api/promise';


Any reason this is not the original import? (This has been done in a couple of places now). This example use is actually great since it shows the 2 side-by-side, so being a bit more explicit as it was is a bit cleaner and clearer.

I just changed the naming for consistency based on this review comment #292 (comment).

I agree that it would be better to show more explicitly here though since users can compare them side-by side.

I've pushed a commit reverting back

jacogr · 2018-12-08T08:09:26Z

packages/api/src/Base.ts

@@ -178,13 +178,17 @@ export default abstract class ApiBase<R, S, E> implements ApiBaseInterface<R, S,
   * <BR>
   *
   * ```javascript
-   * api.on('disconnected', () => {
+   * api.on('connected', () => {


jacogr · 2018-12-08T08:09:53Z

packages/api/src/promise/index.ts

@@ -36,70 +36,98 @@ import SubmittableExtrinsic from './SubmittableExtrinsic';
 * <BR>
 *
 * ```javascript
- * import ApiPromise from '@polkadot/api/promise';
+ * import Api from '@polkadot/api/promise';


Not 100% sure why it has been changed?

jacogr · 2018-12-08T08:13:27Z

packages/api/src/promise/index.ts

 * ```
 * <BR>
 *
 * Submitting a transaction -
 * <BR>
 *
 * ```javascript
- * import ApiPromise from '@polkadot/api/promise';
+ * import Api from '@polkadot/api/promise';


I would just remove this and revert to the original here. IT is a bit overwhelming as an in-code comment now. (Examples are great for making these valid). So rather -

revert to original

import keyring (can just do the test one here, with an appropriate comment)

leave rest as-is

jacogr · 2018-12-08T08:14:47Z

packages/api/src/rx/index.ts

 *     const elapsed = last
- *       ? `, ${timestamp.toNumber() - last}s since last`
+ *       ? `, ${timestampSeconds - last}s since last`


+1 Ahhh, yes, this was pre-change to extending date. (Maybe use .getTime() above, it is more along the line what 90% out there use.

jacogr · 2018-12-08T08:15:17Z

packages/api/src/rx/index.ts

- *   .subscribe((hash) => {
- *     console.log(`submitted with hash ${hash}`);
- *   });
+ * const ALICE_SEED = 'Alice'.padEnd(32, ' ');


As per the previous in-code example, too unwieldly now :(

… functionality as promises example

… back-to-back example

…mit tx example and check it works

…) instead. Remove unnecessary use of provider

ltfschoen · 2018-12-08T23:22:03Z

@jacogr I've pushed commits that address review comments, merge latest from master, and bump versions in examples

jacogr · 2019-01-18T02:11:52Z

Closing, replaced by #600 (which tracks current interfaces closer)

polkadot-js-bot · 2021-06-08T23:43:16Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

ltfschoen added 27 commits October 15, 2018 00:52

docs: Split Promise Examples into both ApiPromise and ApiRx versions

0e17355

docs: Update Promise Examples 05 and 07 to use Keyring Testpairs. Add…

5b3a012

… Node Interface comment

docs: Update API Readme with Capitalised comments. Add Node Interface…

b15dc69

… comments. Use Yarn. Fix typos. Show equivalence

docs: Update RPC Provider Readme with Capitalised comments. Fix varia…

e431b1f

…ble name

fix: Fix typos in Combined.ts

fd5def2

fix: Fix example since should be connected instead of duplicate disco…

b56ee7a

…nnect. Add missing on ready

docs: Capitalise comments. Update Typedocs to match Example. Rename A…

5dd3f16

…pi to ApiPromise

docs: Capitalise comments. Update Typedoc to match Example. Rename Ap…

5070caa

…i to ApiRx. Add Node Interface comment

docs: Rename Api to ApiPromise in tests

625db54

docs: Rename Api to ApiRx in tests

ccf19da

test: Update test to use Keyring testPairs

5ea03d7

docs: Rename Api to ApiPromise

a75ddfb

docs: Rename provider to wsProvider. Add missing semicolon

a42c284

test: Change to usual HTTP and WS ports used in test

a179487

docs: Rename provider to wsProvider

030b175

refactor: Remove unnecessary blank lines

6fd871b

docs: Capitalise comment

7466785

docs: Fix typos

4fd5d3b

docs: Fix typo

49c7fde

docs: Fix typo

19d9c56

fix: Add missing semicolons

745fdb2

docs: Fix typo

fe6c5a5

docs: Fix typo

233a41c

refactor: Remove unnecessary blank line. Add missing semicolon

720dbde

docs: Fix typo

4e75e8e

docs: Fix typo

f397a54

fix: Change overlooked variable that was renamed

d3120f1

ltfschoen changed the title ~~refactor: Miscellaneous changes~~ refactor: Miscellaneous changes for consistency Oct 14, 2018

ltfschoen requested a review from jacogr October 14, 2018 23:30

jacogr requested changes Oct 15, 2018

View reviewed changes

jacogr previously requested changes Dec 4, 2018

View reviewed changes

ltfschoen added 12 commits December 4, 2018 15:16

review-fix: Remove useless comment

9bad353

review-fix: Use single quotes instead of backticks in non-interpolate…

492d063

…d console.logs

review-fix: Remove unncessary preceding space

9044d20

review-fix: Remove fixme as behaviour is fine

bc18121

review-fix: Remove extra config flag in package.json that ran tests i…

4157484

…n docs/examples

review-fix: Restore usage in ApiPromise instead of link to docs

b17aab5

review-fix: Use Api instead of ApiPromise or ApiRx with ES6 except in…

e9077c0

… docs/examples

review-fix: Restore usage examples after fixing them so they work

0c24ad4

review-fix: Revert so not overlapping ports

506b0a0

review-fix: Rename wsProvider to provider

447021e

merge latest from master

8cc984a

bump versions

9e33526

jacogr requested changes Dec 8, 2018

View reviewed changes

ltfschoen added 8 commits December 8, 2018 14:03

review-fix: Remove FIXME since it works even though not entirely same…

23e087d

… functionality as promises example

review-fix: Revert to explicit ApiPromise and ApiRx instead of Api in…

fbe0eb2

… back-to-back example

review-fix: Revert to previous Api import syntax. Restore promise sub…

5e52a61

…mit tx example and check it works

review-fix: Remove use of moment.js to get unix time and use getTime(…

8622a9f

…) instead. Remove unnecessary use of provider

review-fix: Update JSDoc rx 07 example to use testPairs and simpler

61c696e

merge latest from master

62cffef

docs: Change text since we are showing validator balances

c3ae137

bump versions for examples

84a0ff9

jacogr closed this Jan 18, 2019

jacogr deleted the luke-refactor-misc branch January 18, 2019 02:11

polkadot-js locked as resolved and limited conversation to collaborators Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Miscellaneous changes for consistency #292

refactor: Miscellaneous changes for consistency #292

ltfschoen commented Oct 14, 2018

jacogr left a comment

jacogr Oct 15, 2018

jacogr Oct 15, 2018

ltfschoen Oct 15, 2018

jacogr Oct 15, 2018

jacogr left a comment

jacogr Dec 4, 2018

jacogr Dec 4, 2018

jacogr Dec 4, 2018

jacogr Dec 4, 2018

jacogr Dec 4, 2018

jacogr Dec 4, 2018

jacogr Dec 4, 2018

jacogr Dec 4, 2018

jacogr Dec 4, 2018

jacogr Dec 4, 2018

ltfschoen commented Dec 4, 2018

jacogr Dec 8, 2018

jacogr Dec 8, 2018

ltfschoen Dec 8, 2018

jacogr Dec 8, 2018

jacogr Dec 8, 2018

jacogr Dec 8, 2018

jacogr Dec 8, 2018

jacogr Dec 8, 2018

ltfschoen commented Dec 8, 2018

jacogr commented Jan 18, 2019

polkadot-js-bot commented Jun 8, 2021


		## tests

		1. [Run a local Substrate chain node](https://github.com/paritytech/substrate#61-hacking-on-substrate)

		@@ -0,0 +1,49 @@
		const { combineLatest } = require('rxjs');
		const { first, switchMap } = require('rxjs/operators');

refactor: Miscellaneous changes for consistency #292

refactor: Miscellaneous changes for consistency #292

Conversation

ltfschoen commented Oct 14, 2018

jacogr left a 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

jacogr left a 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

Choose a reason for hiding this comment

ltfschoen commented Dec 4, 2018

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

ltfschoen commented Dec 8, 2018

jacogr commented Jan 18, 2019

polkadot-js-bot commented Jun 8, 2021