Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

executor docs refresh #9446

Closed
wants to merge 2 commits into from
Closed

executor docs refresh #9446

wants to merge 2 commits into from

Conversation

gilescope
Copy link
Contributor

No code changes.

Removed duplication between module docs and readme.md. Apparently this is the 'right' way to do it these days.

Please dive in with any suggestions on how we can re-word the docs to make anything clearer.
(And also if I've misunderstood anything and am twisting the original docs meaning)

@gilescope gilescope added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 27, 2021
@gilescope gilescope requested a review from expenses July 27, 2021 15:30
expenses
expenses approved these changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@expenses expenses left a comment

Choose a reason for hiding this comment

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

I think this looks good, I haven't checked over the executor code to see if it matches though.

@gilescope @expenses
Update client/executor/wasmtime/src/runtime.rs
aedda6e 
Co-authored-by: Ashley <ashley.ruglys@gmail.com>
@gilescope
Copy link
Contributor Author

Great build error:
error[E0658]: arbitrary expressions in key-value attributes are unstable
Except they're not:
rust-lang/rust#83366
Hmm and that was a while ago....

bkchr
bkchr suggested changes Aug 2, 2021
View reviewed changes
client/executor/common/src/lib.rs
@@ -16,8 +16,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

//! A set of common definitions that are needed for defining execution engines.

#![doc = include_str!("../README.md")]
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move the docs back here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can see I'm too early somehow for this change. Will do.

Copy link
Member

Choose a reason for hiding this comment

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

We also don't use this anywhere and I'm not sure that stuff like intra doc links would still work. I honestly would not loose such a good feature for having an external file that some editor can understand better.

@stale
Copy link

stale bot commented Sep 1, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 1, 2021
@stale stale bot closed this Sep 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr requested changes

@expenses expenses expenses approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gilescope @bkchr @expenses