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 stack traces for required modules #4035

Merged
merged 5 commits into from
Mar 19, 2020
Merged

Fix stack traces for required modules #4035

merged 5 commits into from
Mar 19, 2020

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Feb 19, 2020

Before:

error: Uncaught Error: illegal token '105' (../Protobufs/steam/steammessages_clientserver_login.proto, line 1)
► <unknown>:94:16
    at illegal (<unknown>:94:16)
    at parse (<unknown>:734:23)
    at process (<unknown>:108:30)
    at <unknown>:183:17
    at fetchReadFileCallback (<unknown>:51:19)
    at file:https:///C:/Users/Nikolai/deno/std/node/_utils.ts:33:26

After:

error: Uncaught Error: illegal token '105' (../Protobufs/steam/steammessages_clientserver_login.proto, line 1)
► file:https:///C:/Users/Nikolai/deno-steam/node_modules/protobufjs/src/parse.js:94:16

94         return Error("illegal " + (name || "token") + " '" + token + "' (" + (filename ? filename + ", " : "") + "line " + tn.line + ")");
                  ^

    at illegal (file:https:///C:/Users/Nikolai/deno-steam/node_modules/protobufjs/src/parse.js:94:16)
    at parse (file:https:///C:/Users/Nikolai/deno-steam/node_modules/protobufjs/src/parse.js:734:23)
    at process (file:https:///C:/Users/Nikolai/deno-steam/node_modules/protobufjs/src/root.js:108:30)
    at file:https:///C:/Users/Nikolai/deno-steam/node_modules/protobufjs/src/root.js:183:17
    at fetchReadFileCallback (file:https:///C:/Users/Nikolai/deno-steam/node_modules/@protobufjs/fetch/index.js:51:19)
    at file:https:///C:/Users/Nikolai/deno/std/node/_utils.ts:33:26

I'm quite new to Rust, so any suggestions on the code will be greatly appreciated.

@ry
Copy link
Member

ry commented Feb 19, 2020

@seishun Hey thanks - looks fine to me. But would you be able to add a test that shows the new output? Follow the example of 001_hello.js in cli/tests/integration_tests.rs

@seishun
Copy link
Contributor Author

seishun commented Feb 19, 2020

Is cli/tests/integration_tests.rs the right place for a test for an std module?

@ry
Copy link
Member

ry commented Feb 19, 2020

@seishun I guess the test could use std... but ideally it will be just a small standalone script. It just needs to generate the stack trace that has been changed. So it should fail on master and be green here.

@seishun
Copy link
Contributor Author

seishun commented Feb 20, 2020

Added a test.

@seishun
Copy link
Contributor Author

seishun commented Feb 20, 2020

I can't reproduce the test failure locally. Seems unrelated.

@bartlomieju
Copy link
Member

@seishun can you rebase this PR to latest master?

@seishun
Copy link
Contributor Author

seishun commented Mar 18, 2020

@bartlomieju I've rebased it, but it seems it wasn't necessary (no conflicts).

cli/js/globals.ts Outdated Show resolved Hide resolved
@ry
Copy link
Member

ry commented Mar 19, 2020

I have checked that the test fails before the change. I'm +1 on this.

@ry ry requested a review from bartlomieju March 19, 2020 14:24
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 8c1c929 into denoland:master Mar 19, 2020
@ry
Copy link
Member

ry commented Mar 19, 2020

@seishun thanks!

@seishun seishun deleted the fix-require branch March 19, 2020 14:43
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
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.

None yet

3 participants