Skip to content
This repository was archived by the owner on May 22, 2025. It is now read-only.

Docs review#72

Merged
0xGabi merged 3 commits intomasterfrom
docs-review
Jun 27, 2020
Merged

Docs review#72
0xGabi merged 3 commits intomasterfrom
docs-review

Conversation

@0xGabi
Copy link
Copy Markdown
Contributor

@0xGabi 0xGabi commented Jun 24, 2020

Note I did a first commit where I included lint to the markdown files. The actual documentation changes are on this commit: 3a32151

@0xGabi 0xGabi requested review from bpierre and eternauta1337 June 24, 2020 00:33
Comment thread api-reference/connect.md
| `ConnectionError` | Gets thrown if the connection fails. |
| Type | Description |
| :--------------------- | :----------------------------------------------------- |
| `ConnectionError` | Gets thrown if the connection fails. |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't have support for Errors yet. Should we include a quick PR to include them or remove this from the documentation for now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we could try to add proper errors (I am thinking in particular about OrganizationNotFound which is very common), and we could remove them just before releasing if we are not ready.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree we should include at least a handler for OrganizationNotFound . But let's do it in a different PR.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 24, 2020

Codecov Report

Merging #72 into master will decrease coverage by 0.10%.
The diff coverage is 30.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage   29.10%   28.99%   -0.11%     
==========================================
  Files          53       53              
  Lines         835      838       +3     
  Branches      135      137       +2     
==========================================
  Hits          243      243              
- Misses        592      595       +3     
Flag Coverage Δ
#unittests 28.99% <30.00%> (-0.11%) ⬇️
Impacted Files Coverage Δ
packages/connect-core/src/entities/Application.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Repository.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Role.ts 0.00% <0.00%> (ø)
...kages/connect-core/src/utils/path/calculatePath.ts 0.00% <0.00%> (ø)
packages/connect-thegraph/src/parsers/repos.ts 92.30% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c52eb92...3b411c5. Read the comment docs.

Comment thread packages/connect-core/src/entities/Role.ts Outdated
@@ -8,149 +8,149 @@ An `Organization` instance represents an Aragon organization and allows to inter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We still don't have support for several of the API methods on this document. Should we remove them from the docs now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure yes, we could have these in a next branch maybe? That branch would contain any code or documentation that is not ready to be released yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this still necessary with our current GitBook setup?

I will go ahead and give a stab to include the few API methods we miss. I believe we already have all the logic we need.

Comment thread SUMMARY.md Outdated
Comment thread SUMMARY.md Outdated
@@ -8,149 +8,149 @@ An `Organization` instance represents an Aragon organization and allows to inter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure yes, we could have these in a next branch maybe? That branch would contain any code or documentation that is not ready to be released yet.

Comment thread api-reference/organization.md Outdated
Comment thread api-reference/repository.md Outdated
Comment thread api-reference/role.md Outdated
Comment thread api-reference/transaction-intent.md Outdated
Comment thread api-reference/transaction-intent.md Outdated
Comment thread packages/connect-core/src/entities/Role.ts Outdated
Comment thread SUMMARY.md Outdated
Comment thread SUMMARY.md Outdated
@0xGabi 0xGabi merged commit 153ac93 into master Jun 27, 2020
@0xGabi 0xGabi deleted the docs-review branch June 27, 2020 21:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants