Move common type guards into core-util#22872
Conversation
A few type guards had duplicate definitions in multiple packages. This change moves the common functions to `core-util`. Fixes Azure#21734
dgetu
requested review from
AikoBB,
EmmaZhu,
FlorianBorn71,
HarshaNalluru,
KarishmaGhiya,
LuChen-Microsoft,
XiaoningLiu,
bterlson,
deyaaeldeen,
jeremymeng,
jiacfan,
joheredi,
ljian3377,
maximrytych-ms,
mikeharder,
petrsvihlik,
praveenkuttappan,
schaabs,
timovv,
vinjiang,
witemple-msft and
xirzec
as code owners
ghost
added
Azure.Core
App Configuration
ghost
added
Service Bus
Storage
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
joheredi
left a comment
joheredi
left a comment
There was a problem hiding this comment.
Thanks for taking this work @dgetu! The PR is looking great! The only thing I am not too sure about and would love to see removed from the PR, are the changes under /common.
Other than that it would be really cool if you can write a few unit tests for the typeguards in core-util. You can take a look at these tests to see how they are setup in core-util
| @@ -0,0 +1,10 @@ | |||
| { | |||
There was a problem hiding this comment.
I'm not sure about these changes under common/*. Is it possible to remove these changes?
There was a problem hiding this comment.
This looks like the rush change feature that we don't use (but the CADL team does, which is the only reason I know about it.)
sdk/core/core-auth/package.json
Outdated
| "dependencies": { | ||
| "@azure/abort-controller": "^1.0.0", | ||
| "@azure/core-util": "^1.0.1", | ||
| "@azure/dev-tool": "^1.0.0", |
There was a problem hiding this comment.
Can we remove this dev-tool dependency?
|
It may be good to add a Changelog.md entry in @azure/core-util with these changes |
deyaaeldeen
left a comment
deyaaeldeen
left a comment
There was a problem hiding this comment.
Looks great! Always in favor of deleting duplicate code 😄
I left one minor comment.
sdk/test-utils/recorder/package.json
Outdated
| "@azure/core-rest-pipeline": "^1.1.0", | ||
| "@azure/core-auth": "^1.3.2", | ||
| "@azure/core-util": "^1.0.0", | ||
| "@azure/core-util": "^1.0.1", |
There was a problem hiding this comment.
I think it may be because core-util 1.0.0 doesn't have the new functions so code using it will break if 1.0.0 is installed which meets the ^
There was a problem hiding this comment.
Yeah but I don't see the recorder using any of the new stuff.
There was a problem hiding this comment.
I added a few unnecessary dependency bumps and missed this one when I reverted them. Fixed!
This reverts commit 7771acb.
… core/move-typeguard-utils-into-core-util
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
Great stuff! I'm happy to get these consolidated
|
Seeing some odd build failures in
Not sure at all how what changes in the files which looks like just whitespace could have affected this, but since it doesn't appear that there are any |
Yeah lets remove changes in common and storage. I guess a quick |
… core/move-typeguard-utils-into-core-util
… core/move-typeguard-utils-into-core-util
objectHasProperty fails at runtime when it is passed a null or undefined value. This change causes these inputs to instead return false
joheredi
left a comment
joheredi
left a comment
There was a problem hiding this comment.
Looks awesome :) Thanks for the great tests
dgetu
changed the title
| import { isDefined, isObjectWithProperties, objectHasProperty } from "../../src/index"; | ||
| import { assert } from "chai"; | ||
|
|
||
| describe("Type guards", function () { |
|
@dgetu: Should the definitions of I'm not sure about |
6 participants
A few type guards had duplicate identical definitions in multiple packages. This change moves the common functions to
core-util.Fixes #21734