This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
docs: Update Readme with TOC, Contributor Guideline. Update Cargo package descriptions #10652
docs: Update Readme with TOC, Contributor Guideline. Update Cargo package descriptions #10652
Changes from 15 commits
39b5ae9
4dd7577
950d634
11834e3
f91f26d
5892bca
dfacb82
a37742e
049135e
10205c9
cf05a7d
bc30b00
a8132d7
8124328
e6e5f82
c7f2a87
8a139c8
282a4e3
c40158c
d1ec475
e7ac760
b820960
b9f6384
a8fb47d
cdaa2a7
c1163ef
851499d
e9e3920
8d0e013
95dd2cb
e87b8ba
68578b4
39cdd6f
55d1a3e
d880c01
46b5e94
c1a71f2
4a85e6c
805e39d
c095637
9f69b61
b89af4f
68d0f15
488d9d7
8bfadc0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even allow pushing directly to master? We shouldn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit b9f6384
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I think this line is redundant then - if you can't push to master you can't
--force
push to master either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in another commit c1a71f2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that it's too far going, you can choose whatever branch name you want and you can also use your own fork. The rule should be that you produce a PR, I don't think anyone cares how exactly.
I think the actual rule rather is that branches should be ephemeral, we (used to) wipe out the stale ones periodically from the main repo. Having a name moniker in the branch helps inquiring a person about their unfinshed work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit a8fb47d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't up to date. I think we should either be linking to the Substrate one, or update this one to match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update the wiki to match the substrate one 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/paritytech/wiki/pull/332 just copy-paste?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a potential issue is that we would have the same coding style guide stored in 3x places, each with the same contents:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a new coding style guide that's "general" across all Parity products (i.e. https://wiki.parity.io/Parity-Style-Guide.html), and then the https://wiki.parity.io/Substrate-Style-Guide.html would extend it with Substrate-specific aspects, and link to the "general" guide, and we'd do the same with http://wiki.polkadot.network/en/latest/polkadot/build/rust-style-guide/ (Polkadot-specific aspects), and also do the same with https://wiki.parity.io/Coding-guide (Parity Ethereum-specific aspects), and possibly rename "Coding-guide.md" to "Parity-Ethereum-guide" or similar..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HCastano i've pushed a commit that updates the link so it points to https://wiki.parity.io/Parity-Ethereum-Style-Guide, which works since https://github.com/paritytech/wiki/pull/332 has been merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have concerns with this whole paragraph. The process it describes is not the one we use. No mention of the two-reviewers requirement (even for insubstantial PRs currently), and perhaps particularly concerning: it seems like it allows merging with two positive reviews even though there's a third reviewer that is strongly opposed. I don't think we want to end up in a situation in which we are forced to merge something some of us do not agree with. Such cases are rare (ever happened?) but if/when they do, forcing a merge by appeal to these 48-hour rules etc seems like asking for trouble. Please reconsider this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit b820960
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 48 hours grace period seems arbitrary, I don't see why it should even be mentioned, cause it's the first time I heard of something like this.
The faster we get PRs reviewed and merged the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this in commit b820960
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just say that breaking changes should be carefuly reviewed and tagged as such so they end up in changelog. I don't think we have a rule that a feature-reducting PR should never be approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit c1163ef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the default instruction should be to build all docs right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thanks. I've addressed this in commit 95dd2cb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this section is that useful, would rather see it in some top-level code organisation overview section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's handy if you just want to copy/paste the relevant docs that you want to open in development (without opening them all), but it does take up a lot of space. I've wrapped it in a "Details >" section
I've made changes in commit 55d1a3e
What are you envisioning by a top-level code organisation overview section?
I think ideally there'd be a Parity Ethereum equivalent of https://docs.substrate.dev/docs/architecture-of-a-runtime, where you could just click on one of the packages and it'd take you to where it's published on crates.io or similar.
And that there'd be an equivalent of say https://docs.substrate.dev/docs/srml-overview#section-srml-architecture that contains your architecture diagram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks really awesome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a fair label for what these are. These are not really "utilities": they are core pieces of functionality while "utility" sort of implies a "nice to have" piece of code. Maybe "Parity core libs"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit e87b8ba
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not teach the reader Rust, that is assumed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit 39cdd6f but removing that line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not teach the reader Rust, that is assumed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit 39cdd6f but removing that line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"where to generate" sounds odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit 39cdd6f by rewording it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#![deny(missing_docs)]
should be required in all new code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit 39cdd6f but adding what you've suggested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, don't teach people Rust. Please remove this paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in commit 39cdd6f by removing that paragraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add
[docs here](https://………)
to all of these.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a link to the rustdocs that are on crates.io only for ethabi in commit 46b5e94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on discussion with @tomusdrw in order to be able to add publish documentation links to crates.io for the other internal toolchains evmbin, ethstore, ethkey, and whisper, we first need to "fix" and publish their dependencies. We need to create a separate issue for this. By "fix" we want them to be as minimal as possible, so ethkey shouldn't bring half of parity-ethereum repo. We need to carefuly select packages that are going to be published.It also requires changes in the code to declare dependencies in a different way, i.e.:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created the new issue here: #10739
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we should use relative paths to projects within the repo. I'd also group tools from external repos together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in these commits 55d1a3e, d880c01