-
Notifications
You must be signed in to change notification settings - Fork 185
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
remove unused exports? #2051
base: main
Are you sure you want to change the base?
remove unused exports? #2051
Conversation
> npx ts-unused-exports tsconfig.json src/**/*.js src/**/*ts test/**/*.js test/**/*.ts
@@ -14,7 +14,7 @@ const weekdayFormat = memoize1((locale, weekday) => { | |||
return new Intl.DateTimeFormat(locale, {timeZone: "UTC", ...(weekday && {weekday})}); | |||
}); | |||
|
|||
export function formatNumber(locale = "en-US") { | |||
function formatNumber(locale) { |
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.
here I learned that we don't need to set the default locale (cf. #384) in this function, since it's only called from formatAuto.
@@ -50,12 +50,10 @@ export const singleton = [null]; // for data-less decoration marks, e.g. frame | |||
export const field = (name) => (d) => d[name]; | |||
export const indexOf = {transform: range}; | |||
export const identity = {transform: (d) => d}; | |||
export const zero = () => 0; |
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.
this seems to be here for consistency only
export const one = () => 1; | ||
export const yes = () => true; | ||
export const string = (x) => (x == null ? x : `${x}`); | ||
export const number = (x) => (x == null ? x : +x); | ||
export const boolean = (x) => (x == null ? x : !!x); |
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.
this seems to be here for consistency only
@@ -225,11 +223,6 @@ export function range(data) { | |||
return r; | |||
} | |||
|
|||
// Returns a filtered range of data given the test function. |
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.
unused
export function ibetainv(p, a, b) { | ||
function ibetainv(p, a, b) { |
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.
this file is an external library, we might not want to mess with it too much
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.
I like removing the unused code, but I think we should keep the exports in most cases for consistency — for things that are conceptually “internally public” (i.e., intended to be used in other modules, even if they aren’t yet).
I obtained the list with:
> npx ts-unused-exports tsconfig.json src/**/*.js src/**/*ts test/**/*.js test/**/*.ts
Not sure we need to remove all of this? Some things seem to be here for consistency.