Skip to content
This repository was archived by the owner on Aug 27, 2024. It is now read-only.

Conversation

@JoshOrndorff
Copy link
Contributor

Aims to solve #284

@shawntabrizi
Copy link
Contributor

@JoshOrndorff sorry if updating branch messed with anything on your side. I just pressed the button not thinking about your dev environment

Copy link
Contributor Author

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Looking really good. I'll put some work into finishing the recipe on weights. I've made some comments and suggestions. Here are a few other questions not associated with any particular line.

  • I thought we agreed on standard line length of 120 chars? I don't feel strongly about it, just want to be on the same page.
  • I like the decision to split weights and fees into two separate articles.

consumes, it should manually prevent restrict certain dispatches through any
means that makes sense to that particular chain. An example of such dispatches
in substrate is the call to a smart contract (todo: link when this is
implemented).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawntabrizi What are your thoughts on handling things like this. It will be a good link but isn't ready yet, and probably isn't worth holding up this article.

One option: Edit the todo out, and create an issue to insert the link when ready.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like the right thing to do. Although, I still consider the "next" docs to be a roughish playground.

I am not against merging to them with a TODO, and then doing a final pass to clean up TODOs before we make it the release version of our docs.

I have been doing this with the NEXT STEPS because its hard to know all the things we can reference to, until we write all the docs and make recipes and stuff.

Copy link
Contributor

@joepetrowski joepetrowski Oct 31, 2019

Choose a reason for hiding this comment

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

Let's put TODOs inside <!--html comment tags--> so that we can search for them but they don't show up on the site.

new one from scratch. The latter is outside the scope of this document and is
explained int he dedicated [`Weight`]() conceptual document.

### Using the Default Weight
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have this section at the top, like:

Here is the default simple way that fees look like and you can use them....

In my opinion, the section you start with:

The fee to include a transaction consists of three parts:

Would be even more clear after the user has already seen a simple example of what is going on.

If you want, I can make a suggested refactor here and see if you like it
Then you can break down the details of how they work and the structures behind them.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I would leave this open and we can apply it at the end

Copy link
Contributor

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Honestly, docs are very good and full of detail.

My biggest feedback is that there is a lot of casual comments and verbose language when we can be more precise and concise.

That being said, I would be pretty happy to see this merged in, as it is a net positive in terms of information, and iterate on the style of content in another pass.

Are we waiting for additional content or is it all here?

The entire substrate runtime module library is already annotated with a simple and fixed weight
system. A user can decide to use the same system or implement a new one from scratch. The latter is
outside the scope of this document and is explained int he dedicated [`Weight`]() conceptual
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should bring your explanation into this document, since it makes sense as a developer doc.


A simple example of using this enum in your runtime is:

```rust
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer that we use subheadings and separate out these different kinds of fees.

Then it would show up in table of contents, and would allow direct linking.

Comments can become normal text at this point. I can make this change at a later point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get the suggestion: make the code a subheading, or A simple example of using this enum in your runtime is: a new one?

yeah I don't mind you or anyone applying changes. I did my best to put the content right and if it needs reshaping then it is fine.

@joepetrowski
Copy link
Contributor

Agree with @shawntabrizi that the implementation details put this at risk of going out of date. They are helpful as examples but should be framed as such, and not "this is how it works."

@kianenigma
Copy link
Contributor

Agree with @shawntabrizi that the implementation details put this at risk of going out of date. They are helpful as examples but should be framed as such, and not "this is how it works."

actually I started this PR with only linking to example module and no code and at some point we had a talk with shawn that guided me toward adding a bit more code. So I am not sure about what's the general approach here.

@shawntabrizi: I see that the weight conceptual has a bit more code than you ideally want. But I really like the overall structure of it. and I don't really like separating them more. I can more the section ## Custom Weight Implementation too fees.md maybe. But overall I am starting to get more confused rather than guided by the distinction of what is a developer doc and what is conceptual, so you can just force me here to do it in a certain way.

@kianenigma
Copy link
Contributor

kianenigma commented Oct 31, 2019

I recommend: lets merge this and then Joshy can make sure the code-y part of weigths.md is covered in his JoshOrndorff/substrate-recipes#62 (it is very very similar). and I suppose then we can strip it down. But for now I still like it, since it is the only in-depth doc that tells to you to the deepest bare-bone level, how to write your own custom weight struct (not to be mistaking with using one). We need that somewhere. Actually I think now that this is best to be in weight conceptual rather than a recipe. It is deep stuff about weight that most high level developers won't need, it just happens to have code. I think it is perfectly fine. I am hence signing this implicitly off.

@kianenigma
Copy link
Contributor

kianenigma commented Oct 31, 2019

okay about the last final sections.
Next step
Learn more
Examples
Refernces https://imgflip.com/i/3ev70v

Every single item that I placed in either of these could also be a fit for the rest. a bit confusing. specially learn-more and references usually mean the same imo.

@joepetrowski joepetrowski marked this pull request as ready for review October 31, 2019 14:04
@JoshOrndorff
Copy link
Contributor Author

LGTM. I can't approve because I opened this PR even though @kianenigma wrote most of it.

@shawntabrizi
Copy link
Contributor

@kianenigma I made some pretty large changes to the structure, but not really the content. Will merge in, and you can yell at me if you hate what I did :)

@shawntabrizi shawntabrizi merged commit 516e664 into source Nov 7, 2019
@shawntabrizi shawntabrizi deleted the joshy-kian-fees branch November 7, 2019 00:48
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.

6 participants