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

WebAssembly Module #204

Merged
merged 10 commits into from
May 9, 2023
Merged

WebAssembly Module #204

merged 10 commits into from
May 9, 2023

Conversation

Yongbeom-Kim
Copy link
Contributor

@Yongbeom-Kim Yongbeom-Kim commented Apr 11, 2023

Description

Webassembly module for SA

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested in separate repo, here

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

Hi! Just a minor comment, thanks for the hard work, looks good!

Copy link
Member

Choose a reason for hiding this comment

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

Could you change the line-encoding of this file to CRLF instead of LF, in order to match the rest of the codebase?

It's a bit weird, but this repository uses CRLF encoding for files, even on Unix systems.

To change the line encoding, open the file on VSCode, the on the bottom right toolbar:

image

Click "Select End of Line Sequence" and change it to CRLF, then commit the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!

I've changed the relevant settings on VS Code and fixed all eslint issues, which should have fixed the problem? I'm not entirely sure if the problem is gone, but eslint isn't giving me any warnings so I take it that the issue is fixed.

Please let me know if there's anything else (or if the issue persists)!

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 think the line-encoding is updated in the GitHub repository, it is only updated on your local files. You can check if there are warnings by refering to the GitHub Checks.
Screenshot 2023-04-14 141923
I think your git config core.autocrlf is set to true, so git will replace CRLF with the line encoding in the GitHub repository automatically, which is LF. Which is why it is not updated on the GitHub repository, even if you change it locally.

I faced a similar problem with the line-encodings, but I was able to fix it by referring to this: https://stackoverflow.com/questions/1967370/git-replacing-lf-with-crlf

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point someone configured the repository to use CRLF instead of LF (I believe it was @Cloud7050) within the eslint configuration, I've since left that as is. If we want to move back to LF it would be some minor changes to the build system

Copy link
Contributor

Choose a reason for hiding this comment

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

Using git ls-files --eol, this repo appears to sometimes use LF and sometimes use CRLF in the index (and some files are even mixed). For comparison, the main js-slang repo appears to mainly use LF.

This repo's base eslint config is based on a config I've been carrying with me for a while. As I'm on Windows, that rule's setting is less of a concious choice for this repo. If I'm understanding this correctly, maybe it'll be better to:

  • Rely on each user's git core.autocrlf config to handle line endings across platforms (true on Windows, input on Linux/macOS?)
  • Turn off the linebreak-style rule in the base eslint config. Users are free to locally checkout LF or CRLF
  • Push a commit that converts all existing files to be indexed with LF (could use something like eol-converter-cli on npm)

From there I think the repo may sometimes get CRLF introduced, but they would sometimes get corrected to LF down the line. Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the help with the line-encoding issue - I've fixed the issue with this PR, let me move this discussion to a fresh issue. #209

Copy link
Member

@martin-henz martin-henz Apr 26, 2023

Choose a reason for hiding this comment

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

Is this PR ready to be merged? @Yongbeom-Kim

@RichDom2185
Copy link
Member

I think we can merge this module in. @Yongbeom-Kim could you kindly resolve the conflicts? Thanks!

@Yongbeom-Kim
Copy link
Contributor Author

Sorry for the late follow-up on this, PR is ready to be merged!

@Yongbeom-Kim Yongbeom-Kim mentioned this pull request May 9, 2023
12 tasks
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@RichDom2185 RichDom2185 merged commit 5b3b516 into source-academy:master May 9, 2023
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.

6 participants