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.

Sign up for GitHub

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

Jump to bottom

Null-check call result rather than function itself #7468

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Null-check call result rather than function itself #7468

merged 1 commit into from
Apr 1, 2020

Conversation

amcasey
Copy link
Contributor

@amcasey amcasey commented Mar 31, 2020

From the comment, it seems like this was supposed check for chords, rather than checking for the availability of isChord.

I tested only by compiling the TypeScript code, since I don't actually know what Theia is/does.

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto
vince-fugnitto reviewed Apr 1, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for your first contribution @amcasey!
Do you mind adding a sign-off to your commit and also making sure to sign the Eclipse Contributor Agreement (ECA).

paul-marechal
paul-marechal approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Nice catch! But I can't help but wonder how you managed to find this?

@amcasey
Copy link
Contributor Author

amcasey commented Apr 1, 2020

@marechal-p Consider upgrading to typescript@latest 😉

@vince-fugnitto
Copy link
Member

Nice catch! But I can't help but wonder how you managed to find this?

I'm not sure as to why it did not fail the build?
cc @akosyakov would you perhaps know?

@paul-marechal
Copy link
Member

@amcasey :'(

TS 3.5.1 vs TS 3.8.3 (I think we happen to use such types...)

@amcasey
Copy link
Contributor Author

amcasey commented Apr 1, 2020 •
edited
Loading

@marechal-p I believe we tried to fix that in 3.9 and had to roll it back because it wasn't ready yet (microsoft/TypeScript#37610). You may, nevertheless, find it valuable to switch versions locally and tidy up the other things it finds (I was seeing a lot of strictNullChecks failures).

@amcasey
Copy link
Contributor Author

amcasey commented Apr 1, 2020

@vince-fugnitto I think I've completed all the legal stuff

@amcasey
Copy link
Contributor Author

amcasey commented Apr 1, 2020 •
edited
Loading

@DanielRosenwasser What's the latest on Promise.All?

@akosyakov akosyakov added the dependencies pull requests that update a dependency file label Apr 1, 2020
@akosyakov
Copy link
Member

I'm not sure as to why it did not fail the build? cc @akosyakov would you perhaps know?

@vince-fugnitto because we don't use latest typescript, see #7468 (comment)

@akosyakov
Copy link
Member

Integration tests are failing. Maybe expectations are wrong. @marechal-p Could you check please? If not I can check later.

@akosyakov akosyakov added the electron issues related to the electron target label Apr 1, 2020
@vince-fugnitto vince-fugnitto mentioned this pull request Apr 1, 2020
Closed
@vince-fugnitto
Copy link
Member

@vince-fugnitto I think I've completed all the legal stuff

I confirmed, it looks like the ECA has been successfully signed using the validation tool:

Screen Shot 2020-04-01 at 9 30 38 AM

@paul-marechal paul-marechal force-pushed the patch-1 branch 4 times, most recently from a7c47af to 8015507 Compare April 1, 2020 16:29
@paul-marechal
Copy link
Member

paul-marechal commented Apr 1, 2020 •
edited
Loading

I just learned that I should not have force-pushed on your own repo... My intention was to fix the test to not trouble you too much, but this went south fast... Now I know.

edit: fixed authorship, this was weird.

@akosyakov
Copy link
Member

akosyakov commented Apr 1, 2020 •
edited
Loading

@marechal-p You can rewrite a commit message in accordance with and we merge your commit:

Additional authors can be added using Also-by or Co-authored-by entries, by replacing the name "Some Bodyelse" by the real name of the person

From https://www.eclipse.org/projects/handbook/#resources-commit

@paul-marechal paul-marechal force-pushed the patch-1 branch 4 times, most recently from 7f9f2a4 to 37def7c Compare April 1, 2020 17:16
@amcasey @paul-marechal
Null-check call result rather than function itself
44eb6c0 
From the comment, it seems like this was supposed check for chords, rather than checking for the availability of `isChord`.

Co-authored-by: Paul Maréchal <paul.marechal@ericsson.com>
Signed-off-by: Andrew Casey <andrew.casey@microsoft.com>
@paul-marechal
Copy link
Member

Ok, now the ECA looks ok, it wasn't like this before. Thanks @akosyakov.

@akosyakov
Copy link
Member

ok, merge

@paul-marechal paul-marechal merged commit 5d20fac into eclipse-theia:master Apr 1, 2020
@paul-marechal
Copy link
Member

Was waiting for CI, but it was green just earlier indeed.

@amcasey
Copy link
Contributor Author

amcasey commented Apr 1, 2020

@marechal-p No worries, GH makes that experience super confusing. In any case, my fork exists just for this change, so there's no harm done. 😄

Thanks, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@paul-marechal paul-marechal paul-marechal approved these changes

Assignees
No one assigned
Labels
dependencies pull requests that update a dependency file electron issues related to the electron target
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amcasey @vince-fugnitto @paul-marechal @akosyakov