chore: refactor envinfo #1025
chore: refactor envinfo #1025
Conversation
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
This is great! Let's iterate on the typings so we don't have to cast
|
|
||
| beforeAll(async () => { | ||
| environmentInfo = await getEnvironmentInfo(); | ||
| environmentInfo = (await getEnvironmentInfo()) as EnvironmentInfo; |
There was a problem hiding this comment.
This is not good. Let's type getEnvironmentInfo properly so it returns correct type based on the arguments
molant
force-pushed
the
refactor/envinfo
branch
4 times, most recently
from
2773aba to
6fb73a2
Compare
packages/cli/src/tools/envinfo.ts
Outdated
| * If set to `false`, it will be a `string`. | ||
| */ | ||
| async function getEnvironmentInfo(): Promise<EnvironmentInfo>; | ||
| async function getEnvironmentInfo(json: boolean): Promise<string>; |
There was a problem hiding this comment.
I'm not super thrilled with this but haven't come up with a better idea yet.
getEnvironmentInfo()return type will beEnvironmentInfo✔getEnvironmentInfo(false)return type will be astring✔getEnvironmentInfo(true)return type will be astring❌
There was a problem hiding this comment.
Got it finally right 😊
molant
force-pushed
the
refactor/envinfo
branch
from
6fb73a2 to
b1b2623
Compare
packages/cli/src/tools/envinfo.ts
Outdated
| ): Promise<string | EnvironmentInfo> { | ||
| const options = json | ||
| ? {json: true, showNotFound: true} | ||
| : {json: false, showNotFound: true}; |
There was a problem hiding this comment.
We can have a single object and use json value because it's already boolean
molant
force-pushed
the
refactor/envinfo
branch
from
b1b2623 to
538041a
Compare
packages/cli/src/tools/envinfo.ts
Outdated
| async function getEnvironmentInfo( | ||
| json = true, | ||
| ): Promise<string | EnvironmentInfo> { | ||
| const options = {json, showNotFound: json}; |
There was a problem hiding this comment.
I thought we always want to show Not Found?
There was a problem hiding this comment.
I'm all for always showing Not Found. Is just that in one place it was explicit and in the other not (and I think the default is false).
I'll update the PR
molant
force-pushed
the
refactor/envinfo
branch
2 times, most recently
from
8af0ba9 to
2bd4513
Compare
|
Thanks! |
I only see a single call site being updated as a part of this PR. Are there any other places we haven't refactored? |
|
This was the only one and it also got out of sync with options, so it's an improvement already. |
|
The other call used |
3 participants
Summary:
While working on #976 I saw that I needed to modify the properties requested to
envinfoand that it was being accessed in a couple different places with different configurations.This PR changes that to use a single point (and single configuration) to get the info.