-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[chore] common-utils module #2773
Conversation
igorDykhta
commented
Nov 20, 2024
•
edited
Loading
edited
- separate common utils module, utils without heavy dependencies
Signed-off-by: Ihor Dykhta <dikhta.igor@gmail.com>
✅ Deploy Preview for keplergl2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Ihor Dykhta <dikhta.igor@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something called common-utils
quickly tends to become a "trash can" of unrelated things.
I do see a bunch of data-type-utils in there, and it seems to wrap type-analyzer in a couple of places which I personally think is a good direction.
Should each module have a README describing what it does?
Should it have any documentation?
Should it have any tests?
@@ -18,6 +18,7 @@ | |||
"workspaces": [ | |||
"./src/types", | |||
"./src/constants", | |||
"./src/common-utils", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I wish we could change the top level src
to modules
or packages
... src/<module>/src
is kind of confusing...
import {range} from 'd3-array'; | ||
import {isHexWkb, notNullorUndefined} from './data'; | ||
|
||
export const ACCEPTED_ANALYZER_TYPES = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can we add a TSDoc comment in front of every type, function, field/member/method?
In this comment maybe state what it would mean for a type not to be accepted.