Skip to content
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

CSUB-256: Document Creditcoin-JS #1348

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from
Draft

CSUB-256: Document Creditcoin-JS #1348

wants to merge 11 commits into from

Conversation

pLabarta
Copy link
Contributor

@pLabarta pLabarta commented Sep 25, 2023

Description of proposed changes

CONSIDER BEFORE MERGING: Until the package is published in NPM, installation guide will be misguiding

This PR:

  • Removes the alpha warning from README
  • Adds a simple installation guide (which asumes creditcoin-js is published on npm unscoped)
  • Adds docs on how to build augmented types
  • Adds usage examples to the README
  • Adds additional working full examples to the example folder

This PR does not:

  • Add testing of the examples to CI/integration-tests
  • Add any logic to publish to NPM

Practical tips for PR review & merge:

  • All GitHub Actions report PASS
  • Newly added code/functions have unit tests
    • Coverage tools report all newly added lines as covered
    • The positive scenario is exercised
    • Negative scenarios are exercised, e.g. assert on all possible errors
    • Assert on events triggered if applicable
    • Assert on changes made to storage if applicable
  • Modified behavior/functions - try to make sure above test items are covered
  • Integration tests are added if applicable/needed

@pLabarta pLabarta marked this pull request as ready for review September 25, 2023 20:33
@codecov-commenter
Copy link

Codecov Report

Merging #1348 (c125492) into dev (d7dddc4) will decrease coverage by 0.66%.
Report is 13 commits behind head on dev.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #1348      +/-   ##
==========================================
- Coverage   70.83%   70.17%   -0.66%     
==========================================
  Files         107      107              
  Lines       12411    12506      +95     
  Branches      120      120              
==========================================
- Hits         8791     8776      -15     
- Misses       3610     3730     +120     
+ Partials       10        0      -10     

see 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

For full LLVM coverage report click here!

@atodorov atodorov marked this pull request as draft September 26, 2023 14:55
@atodorov
Copy link
Contributor

CONSIDER BEFORE MERGING: Until the package is published in NPM, installation guide will be misguiding

Converted to Draft to avoid merging accidentally before we're sure we want to do this (or publish the package to NPM in the mean time).

Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions and change requests added.

Creditcoin-js requires the following to be installed:

- [Node.js](https://nodejs.org/en/)
- [TypeScript](https://www.typescriptlang.org/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we install typescript with yarn ?

I would add yarn as a requirement here tool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about TypeScript being listed as prerequisite? I think it will be installed by yarn so no need to mention it explicitly, no ?

creditcoin-js/README.md Outdated Show resolved Hide resolved
creditcoin-js/README.md Show resolved Hide resolved
const keyring = new Keyring({ type: 'sr25519' });
const alice = keyring.addFromUri('//Alice');

await tx.signAndSend(alice);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit ambiguous (the tx variable I mean). I had to scroll back and read the example above to figure out that both snippets are connected.

Maybe highlight this connection somehow. A big fat warning or a full example with individual sections as comments is the only thing I can think off. The latter having the benefit that we can move it into a file and exercise it via CI.

creditcoin-js/README.md Show resolved Hide resolved
creditcoin-js/README.md Show resolved Hide resolved
creditcoin-js/README.md Outdated Show resolved Hide resolved
creditcoin-js/src/examples/batch.ts Show resolved Hide resolved
**WARNING**: This is an alpha version of creditcoin-js. It is not ready for production use. It will have breaking changes often.
## Getting started

### Preqrequisites
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Preqrequisites
### Prerequisites

import { CreditcoinApi, creditcoinApi } from 'creditcoin-js';
import { transferExample } from 'creditcoin-js/lib/examples/transfer';

describe('creditcoin js examples', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the hard-coded address below I think this only needs to be executed when running against a local network. You can use describeIf and/or testIf from ../utils and make execution conditional based on the URL for example.

Then you can remove the usage of global config values from the example itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants