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

changed fibonacci.wasm to fibonacci.wat #180

Merged
merged 2 commits into from
Oct 26, 2023
Merged

changed fibonacci.wasm to fibonacci.wat #180

merged 2 commits into from
Oct 26, 2023

Conversation

tusharad
Copy link
Contributor

Explanation

In the documentation, the fibonacci.wasm file link leads to 404 page since, Now the directory contains .wat files.

Copy link
Collaborator

alabulei1 commented Oct 26, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall, the pull request changes the file name from fibonacci.wasm to fibonacci.wat in both the documentation and the file links. The potential problems identified include the need to verify the correctness of the fibonacci.wat file and the lack of information about the reason for the name change.

The most important findings are the inclusion of a note about converting the file to wasm using the WABT tool and the potential need for more explicit instructions on this conversion. Additionally, clarification on the reason for the file extension change would be helpful for users familiar with WebAssembly.

In summary, no functional or logical issues are expected due to the name change, but further verification is needed.

Details

Commit 06ae3248a2e05dba08a39fd2d31349de22c520da

Key changes in the pull request are as follows:

  • The file name fibonacci.wasm is changed to fibonacci.wat in multiple places in the latest.md file.
  • The links to the original fibonacci.wasm file are also updated to point to the new fibonacci.wat file.

Potential problems:

  • The patch only includes changes related to file name updates, so there shouldn't be any functional or logical issues introduced.
  • However, it's important to verify that the new fibonacci.wat file is correctly updated and available at the provided link.
  • The patch doesn't include any information about why the name change was made, so it would be useful to have that context.

Commit c3d577149febb4c19ecef5928eec4c8c7f5a5c79

Key Changes:

  • Updated references to the "fibonacci.wasm" file to "fibonacci.wat" in the documentation.
  • Added a note stating that users should convert the provided "fibonacci.wat" file into a wasm file using the WABT tool.

Potential Problems:

  • The added note about converting the "fibonacci.wat" file into a wasm file using the WABT tool is useful information for users, but it may be helpful to provide more explicit instructions on how to perform this conversion.
  • It would be good to clarify why the file extension was changed from ".wasm" to ".wat". This information may be relevant to users who are familiar with WebAssembly.

Overall, the changes seem straightforward and primarily involve updating references to the file name extension.

@tusharad tusharad marked this pull request as ready for review October 26, 2023 14:48
@tusharad
Copy link
Contributor Author

Hi @hydai, Could you please take another look at this PR?

Copy link
Member

@hydai hydai left a comment

Choose a reason for hiding this comment

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

We no longer ship WASM files. So these lines should change to fibonacci.wat and ask users to convert it to WASM via the wat2wasm tool.

@tusharad
Copy link
Contributor Author

tusharad commented Oct 26, 2023

@hydai I have added a note regarding wat2wasm.

Copy link
Member

@hydai hydai left a comment

Choose a reason for hiding this comment

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

LGTM

@hydai hydai merged commit 3991760 into WasmEdge:main Oct 26, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants