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

fix: fix up the terminal output result from null #3987

Merged
merged 4 commits into from
Aug 28, 2023

Conversation

fwx5618177
Copy link
Contributor

Issue: #3975

Result:
image

@netlify
Copy link

netlify bot commented Aug 12, 2023

👷 Deploy request for remixproject pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 332f2f3

@fwx5618177
Copy link
Contributor Author

PTAL. @drafish @Aniket-Engg

@drafish
Copy link
Contributor

drafish commented Aug 12, 2023

看你是中文用户,我就懒得打英文了。大致看了下,没看出啥问题。但不知道为啥自动化测试挂了一大片。可能是你改的那部分有问题,也有可能是 e2e 的问题,要改 e2e 的代码。我得找个时间好好看看。

另外,我在做 Remix 中文社区,我们的 tg 频道是 https://t.me/remix_cc 欢迎加入

@fwx5618177
Copy link
Contributor Author

看你是中文用户,我就懒得打英文了。大致看了下,没看出啥问题。但不知道为啥自动化测试挂了一大片。可能是你改的那部分有问题,也有可能是 e2e 的问题,要改 e2e 的代码。我得找个时间好好看看。

另外,我在做 Remix 中文社区,我们的 tg 频道是 https://t.me/remix_cc 欢迎加入

e2e问题,重新部署下,报错的区域就会变。之前提过的pr都会有这个错

  • Maybe these errors are e2e issues. Re-deploy it, and the location of errors changed.

@fwx5618177
Copy link
Contributor Author

Have tested all error cases, and they could execute success locally. I suspect that Circle tested cases encountered some errors.

@fwx5618177
Copy link
Contributor Author

@joeizang PTAL. Thanks.

@drafish
Copy link
Contributor

drafish commented Aug 15, 2023

最好用 rebase ,不要用 merge 。因为 merge 会产生 merge 记录。你不知道什么时候 remix 会 merge 你的 pr ,你可能会需要经常同步 master 分支的改动,如果一直用 merge ,就会产生很多 merge 记录。而 rebase 就不会有这个问题。在这个页面上就可以直接 rebase ,但是你得选下,默认选项是 merge 。

@drafish
Copy link
Contributor

drafish commented Aug 24, 2023

还有一个问题,你在 commit 的时候没有用 github 账号,这样你的 commit 记录就不会关联到你的 github 账户。将来 remix merge 了你的 PR 后,contributors 名单中出现的就是 moxi ,而不是 @fwx5618177 。最好还是重新用 github 账号 commit 一下。

@fwx5618177
Copy link
Contributor Author

还有一个问题,你在 commit 的时候没有用 github 账号,这样你的 commit 记录就不会关联到你的 github 账户。将来 remix merge 了你的 PR 后,contributors 名单中出现的就是 moxi ,而不是 @fwx5618177 。最好还是重新用 github 账号 commit 一下。

没事,谢谢大兄

// strictly check condition on 0, false, except undefined, NaN.
// undefined: automatically throw "undefined" is not valid JSON
// NaN: read value from msg is `null`
if (msg === false || isNaN(msg) || msg === 0) msg = msg.toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

isNaN(msg) 这里还得处理下,就是这里导致了 e2e 测试失败。我把 isNaN(msg) 删掉以后 e2e 测试就通过了。具体代码逻辑我还没细看,就只是定位了一下 e2e 测试失败的原因。具体应该改这里的代码,还是改e2e测试用例,这个还得 @fwx5618177 你自己判断下

Comment on lines 689 to 691
// strictly check condition on 0, false, except undefined, NaN.
// undefined: automatically throw "undefined" is not valid JSON
// NaN: read value from msg is `null`
Copy link
Contributor

Choose a reason for hiding this comment

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

注释这里还得再改下,因为你已经把 isNaN 去掉了。还有就是注释对齐一下,这是个小细节。另外这里似乎没有对 undefined 做特殊处理吧,没太明白为什么要加这个注释

@drafish
Copy link
Contributor

drafish commented Aug 28, 2023

Hi @ryestew @Aniket-Engg This PR is ready to merge.

@Aniket-Engg
Copy link
Collaborator

Thanks @fwx5618177

@Aniket-Engg Aniket-Engg merged commit d6c063a into ethereum:master Aug 28, 2023
1 check 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.

4 participants