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(cli) fix close websocket with code is not working (recreated) #8776

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

Preta-Crowz
Copy link
Contributor

original pr : #8611
original pr has conflict with #8640 and that was merged first.
so I created this pull request again.

@bartlomieju
Copy link
Member

@crowlKats please take a look

Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju
Copy link
Member

@Preta-Crowz @crowlKats can we get some test case for this PR?

@Preta-Crowz
Copy link
Contributor Author

@bartlomieju
sorry for dirty code
here's test code

const cases = [
    1000, // 1000, valid
    3000, // 3000, valid
    5000, // 5000, invalid
    3141, // in range, valid
    1024, // lower than 3000 and not 1000, invalid
    6000  // higher than 5000 and not 1000, invalid
]

for (const code of cases) {
    console.log(code + " on old code : " +
        ((code && (code !== 1000 && !(3000 <= code > 5000))) ? "\u001b[31minvalid" : "\u001b[32mvalid") + "\u001b[0m\t" +
        code + " on new code : " +
        ((code && !(code === 1000 || (3000 <= code && code < 5000))) ? "\u001b[31minvalid" : "\u001b[32mvalid") + "\u001b[0m"
    );
}

output

1000 on old code : valid        1000 on new code : valid
3000 on old code : invalid      3000 on new code : valid
5000 on old code : invalid      5000 on new code : invalid
3141 on old code : invalid      3141 on new code : valid
1024 on old code : invalid      1024 on new code : invalid
6000 on old code : invalid      6000 on new code : invalid

@Preta-Crowz
Copy link
Contributor Author

colored image of output
image

@Preta-Crowz
Copy link
Contributor Author

I. Hate. Conflicts.

@Skillz4Killz
Copy link

Any ETA on this?

@bleugateau
Copy link

ETA on this ?

Thanks in advance

@crowlKats
Copy link
Member

Once conflicts are resolved and tests are provided

@bleugateau
Copy link

bleugateau commented Feb 7, 2021

Once conflicts are resolved and tests are provided

Yes I know, but I'm talking to author
If she not fix this, I'm gonna make my own PR about this

Have a good day

@Preta-Crowz
Copy link
Contributor Author

I already made this pr once and someone moved files to other folders and I got conflicts. Then I recreated it. I'll fix it soon. Also, changed code was tested.

P.S. I'm not he/him. I'm she/her.

@crowlKats
Copy link
Member

crowlKats commented Feb 7, 2021

Also, changed code was tested.

We need actual test cases. Look at cli/tests/websocket_test.ts

TriForMine added a commit to TriForMine/discordeno that referenced this pull request Feb 10, 2021
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@Preta-Crowz
Copy link
Contributor Author

merged main to patch once because file was moved

@kt3k
Copy link
Member

kt3k commented Feb 12, 2021

@Preta-Crowz Thank you for your fix!

Note: we can add test cases for this fix later. It's tracked in #9001.

@kt3k kt3k merged commit 7f8b44a into denoland:master Feb 12, 2021
ghost pushed a commit to discordeno/discordeno that referenced this pull request Feb 15, 2021
* Adding missing await and updating some deps

* Adding missing await and updating some deps

* Adding missing await and updating some deps

* Fix close code 4009 until deno fixes the issue: denoland/deno#8776

* Fix heartbeating

* Add await for the requestGuildMembers in requestAllMembers

* Change body && body.file to body?.file

* Fix lint #1

* Change body && body.file to body?.file

* Fix lint

* Deno lint

* Update request.ts

* Fix deno lint error

* Update src/ws/shard_manager.ts

Co-authored-by: ayntee <ayyantee@gmail.com>

* Fix fetchMembers

* Fix getMembersByQuery

* Try to fix RequestMembersQueue processing

* Deno lint

* Fix requestGuildMembers

* Fix requestGuildMembers

* Fix requestAllMembers

* Undo useless changes

* Fix merge conflict

* Fix merge conflict

* Change for loop to Promise.all

* Deno fmt

Co-authored-by: ayntee <ayyantee@gmail.com>
Co-authored-by: Skillz4Killz <23035000+Skillz4Killz@users.noreply.github.com>
github-actions bot pushed a commit to discordeno/discordeno that referenced this pull request Feb 15, 2021
* Adding missing await and updating some deps

* Adding missing await and updating some deps

* Adding missing await and updating some deps

* Fix close code 4009 until deno fixes the issue: denoland/deno#8776

* Fix heartbeating

* Add await for the requestGuildMembers in requestAllMembers

* Change body && body.file to body?.file

* Fix lint #1

* Change body && body.file to body?.file

* Fix lint

* Deno lint

* Update request.ts

* Fix deno lint error

* Update src/ws/shard_manager.ts

Co-authored-by: ayntee <ayyantee@gmail.com>

* Fix fetchMembers

* Fix getMembersByQuery

* Try to fix RequestMembersQueue processing

* Deno lint

* Fix requestGuildMembers

* Fix requestGuildMembers

* Fix requestAllMembers

* Undo useless changes

* Fix merge conflict

* Fix merge conflict

* Change for loop to Promise.all

* Deno fmt

Co-authored-by: ayntee <ayyantee@gmail.com>
Co-authored-by: Skillz4Killz <23035000+Skillz4Killz@users.noreply.github.com> 602a745
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.

7 participants