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

[Table/Core] Move some table utils into core #1604

Merged
merged 8 commits into from
Sep 25, 2017

Conversation

cmslewis
Copy link
Contributor

Fixes #1601

Changes proposed in this pull request:

  • 🔄 CHANGED The following packages/table utils now live in packages/core:
    • arraysEqual
    • deepCompareKeys
    • getDeepUnequalKeyValues
    • getShallowUnequalKeyValues
    • shallowCompareKeys

This change is necessary to unblock PR #1492.

Reviewers should focus on:

  • I updated all references to these utils throughout packages/table.
  • I renamed BlueprintUtils to CoreUtils throughout packages/table. Now everything is consistent.
  • I left facades in packages/table/src/common/utils so as to not break the public API; the table utils are exported after all. All those facades include function descriptions identical to the ones in packages/core/src/common/utils, except they include @deprecated warnings.
  • Anyone know how to import interfaces like IKeysWhitelist an IKeysBlacklist from core, and re-export them from table? Tried and failed to do that syntactically, so I just redefined the interfaces in table.
  • Is it possible to use the { utilName: CoreUtils.utilName } pattern if CoreUtils.utilName is generically typed? Couldn't get that to work either, so I spelled out the functions more verbosely.

Copy link
Contributor

@gscshoyru gscshoyru left a comment

Choose a reason for hiding this comment

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

Seems simple enough? I'm not sure on the typings questions, though, I'd need to be playing with it locally myself I think to be able to grok what you're talking about, sorry.

@blueprint-bot
Copy link

Fix build

Preview: documentation | table
Coverage: core | datetime

@cmslewis
Copy link
Contributor Author

@llorca feel free to toss this into this week's release

* Returns true if the arrays are equal. Elements will be shallowly compared
* by default, or they will be compared using the custom `compare` function
* if one is provided.
* @deprecated since 1.26.0; import this function from core Utils instead.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@llorca if this PR makes it into this week's release, I believe this table version would be correct, right?

Copy link
Contributor

@llorca llorca Sep 22, 2017

Choose a reason for hiding this comment

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

Correct! Can we explicitly say @deprecated since @blueprintjs/table 1.26.0 (not sure if we have a pattern for that) to avoid confusion with the generic/core version?

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

how about compareUtils.ts for all these new functions? there's a lot of code there and utils.ts becomes unmaintainably long.

@llorca
Copy link
Contributor

llorca commented Sep 22, 2017

@giladgray why not a utils/ folder

@giladgray
Copy link
Contributor

@llorca we should definitely make a utils/ folder, but not in this PR.

@cmslewis
Copy link
Contributor Author

cmslewis commented Sep 25, 2017

how about compareUtils.ts for all these new functions

@giladgray yes please

@cmslewis
Copy link
Contributor Author

cmslewis commented Sep 25, 2017

@giladgray @llorca though in making compareUtils.ts, I'm struggling with two things:

  1. There are other, existing comparison-oriented functions like arrayLengthCompare and approxEqual. Should those live in compareUtils.ts too?
  2. I'd rather not export a bunch of different *Utils.ts files from Core, but I also can't do something like export * from "./compareUtils.ts" from a common, top-level utils.ts file. We okay with me importing functions by name into utils.ts and then exporting all of them individually? Derp brain fart. Of course you can.

@blueprint-bot
Copy link

Move code into utils/compareUtils.ts

Preview: documentation | table
Coverage: core | datetime

@cmslewis cmslewis dismissed giladgray’s stale review September 25, 2017 20:23

Addressed comments

* Returns true if the arrays are equal. Elements will be shallowly compared
* by default, or they will be compared using the custom `compare` function
* if one is provided.
* @deprecated since @blueprintjs/table 1.26.0; import this function from
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should now change to 1.27.0 since we didn't make last week's release

Copy link
Contributor

@themadcreator themadcreator left a comment

Choose a reason for hiding this comment

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

Looks good.

I would have thought export { IKeysWhiteList } from "..." would have worked.

@cmslewis
Copy link
Contributor Author

@themadcreator fortunately it's easy to play with that without affecting the API now. maybe i'll figure something out after merging.

@cmslewis cmslewis merged commit ae1524c into master Sep 25, 2017
@cmslewis cmslewis deleted the cl/move-compare-keys-utils branch September 25, 2017 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants