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

Several fixes #3

Open
wants to merge 9 commits into
base: gh-pages
Choose a base branch
from
Open

Conversation

corollari
Copy link

  • Added references to the NeoVM opcode reference
  • Clarified some examples
  • Made the use of instructions more consistent across the tutorial
  • Fixed the appearance of generic code when moving up or down in the interactive prompt
  • Other minor fixes

I haven't built the website and tested it because I haven't been able to find the instructions on how to do it.


push1 push2 push3 push4 push2 pick
push1 push2 push3 push4 push1 pick
Copy link
Member

Choose a reason for hiding this comment

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

Why change the example?

Copy link
Author

Choose a reason for hiding this comment

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

With the previous example the stack just remains the same after applying pick, this behaviour confused me a bit when following the tutorial so I decided to change it to an example that modified the stack so other people could appreciate the behaviour of pick better.

Copy link
Author

Choose a reason for hiding this comment

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

Still, it may be easier for some people to follow the previous example so if this change is controversial i can revert it.

Copy link
Member

Choose a reason for hiding this comment

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

With the previous example the stack just remains the same after applying pick

Really? 🤔 I have to check...

Copy link
Author

Choose a reason for hiding this comment

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

Screenshot
After applying pick:
Screenshot


Resulting stack:

{% include stack.html stack="1 2 3 4 2" %}
{% include stack.html stack="1 2 3 4 3" %}
Copy link
Member

Choose a reason for hiding this comment

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

Why change the example? This affected the result. Is it a better one?

Copy link
Author

Choose a reason for hiding this comment

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

Consequence from the change in the example.


Resulting stack:

{% include stack.html stack="1 3 4 2" %}
{% include stack.html stack="1 2 4 3" %}
Copy link
Member

Choose a reason for hiding this comment

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

Why change the example? Is it better?

Copy link
Author

Choose a reason for hiding this comment

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

Changed it to remain consistent after changing the other example.

index.markdown Outdated
@@ -292,23 +292,23 @@ In next sections, we will see stack opcodes that receive parameters from stack.

### `pick` (opcode `0x79`)

`pick` reads `n` and copies `n`-th element to top stack. Example:
`pick` reads and drops the number at the top of the stack, which we will call `n`, and copies the `n`-th element to the top of the stack. Example:
Copy link
Member

@igormcoelho igormcoelho Jul 3, 2019

Choose a reason for hiding this comment

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

Good idea.. I think that changing to pick consumes n could be even simpler. Phrase became too long.

Copy link
Author

Choose a reason for hiding this comment

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

The issue that I came across when following the tutorial is that n is not defined anywhere so you have to end up guessing what it is by looking at the examples. I was trying to fix that with these changes.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps: consumes top-stack value n...

Copy link
Author

Choose a reason for hiding this comment

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

That's a great idea, I'll change it!

@@ -569,11 +569,11 @@ _______________|________________________________|____________
```

To inspect what happened, use the following commands to get array from altstack and pick inside it:
`dupfromaltstack 0 pickitem` (gets first element), `drop`, `dupfromaltstack 1 pickitem` (gets second element).
`dupfromaltstack push0 pickitem` (gets first element), `drop`, `dupfromaltstack push1 pickitem` (gets second element).
Copy link
Member

Choose a reason for hiding this comment

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

Good

@@ -86,7 +86,7 @@ function Editor(selectorOrElement) {
}

function selectPreviousLine() {
if (selectedLine === null || selectedLine === 0) {
if (selectedLine === null || selectedLine === 233) {
Copy link
Member

Choose a reason for hiding this comment

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

Why this fix?

Copy link
Author

Choose a reason for hiding this comment

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

If you go into one of the interactive prompts in the webpage and press the upper or lower arrow in your keyboard a generic forth script is displayed. This prevents that script from being displayed while still preserving the arrow functionality.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it's better to create a clearconsole function, so we clear past commands.

Copy link
Member

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

few changes

@igormcoelho
Copy link
Member

I haven't built the website and tested it because I haven't been able to find the instructions on how to do it.

I haven't done it myself too hahah only used on github.io pages. Must see how to do this locally.

@corollari
Copy link
Author

Hi @igormcoelho, I was going through my open PRs and stumbled upon this. Is there something that I should change on this PR? I can also close it if you consider that it doesn't add enough value.

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