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

Added a fail through check for the project ID #938

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

rfennell
Copy link
Owner

@rfennell rfennell commented Feb 1, 2021

What problem does this PR address?

After merging #pr937 it was found that the process of getting WI/CS details could fail due to a missing project ID.

Code has been added to try two locations for the projects ID

  • The originally used property
  • If this is null then it falls back to the extracting the value from the JSON

Also in the process of debuging this add a bit more logging

@LouisSung
Copy link
Contributor

LouisSung commented Feb 1, 2021

Thank you for the detailed testing and quick check :D

I also review the change, doesn’t it look weird? Why using the && here? :o
Is the blue one correct? Or miss typed in?
A583F4E3-01B6-43BA-B77E-7423FB0EB842

@rfennell
Copy link
Owner Author

rfennell commented Feb 1, 2021

It is a

item to null check && if not null value || if null value

Is there a shorter syntax?

I know enough Node to be dangerous :)

@rfennell rfennell merged commit b944f11 into main Feb 1, 2021
@rfennell rfennell deleted the pr937-fix-for-missing-projectID branch February 1, 2021 16:49
@LouisSung
Copy link
Contributor

Oh you mean the ternary operator? falsyableItem ? truthy : falsy?
It’s different from falsyableItem || fallbackItem

rfennell added a commit that referenced this pull request Feb 1, 2021
@rfennell
Copy link
Owner Author

rfennell commented Feb 1, 2021

Oops I got it from Stackoverflow, and confused two forms. I will tidy that and rebuild

@LouisSung
Copy link
Contributor

LouisSung commented Feb 1, 2021

undefined && undefined || value === (undefined && undefined) || value === undefined || value
Since logic AND (&&) has higher priority comparing to logic OR (||), ref Operator Precedence
It still works but kind of duplicated

@rfennell
Copy link
Owner Author

rfennell commented Feb 1, 2021

Thanks for the guidance, I will make the before I release the update

@LouisSung
Copy link
Contributor

LouisSung commented Feb 1, 2021

I think the xxx || yyy is better than xxx ? xxx : yyy in this situation since the former one is used for fallback and therefore is more readable and correct for the semantic point of view :)

Actually the better choice for null check is Nullish Coalescing (??) introduced in TypeScript 3.7 (this extension is using 3.6 though)
However, I think the || SHOULD be used here because you’d also want to fallback empty string but not just null/undefined

rfennell added a commit that referenced this pull request Feb 1, 2021
* Corrected poor syntax

* Updated the type hint
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.

2 participants