-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Convert to TypeScript #339
Conversation
"main": "build/index.js", | ||
"types": "build/index.d.ts", |
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.
Changed "main" and "types" here to export the Rule
and the Config
types from the package root. The processDatabase
function is exported as before.
/** | ||
* The severity of a rule. `off` means the rule is disabled, `error` means the rule is enabled. | ||
*/ | ||
export type Severity = "off" | "error"; |
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.
In fact, any value other than "error"
can be specified to disable the rule, but I chose the value "off"
to match ESLint. Does it matter?
@@ -1,11 +1,13 @@ | |||
import { Schema } from "extract-pg-schema"; | |||
|
|||
export type Reporter = (p: { | |||
export type Issue = { |
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 named this object Issue
. Doesn't that sound strange? Another idea is Problem
.
export type { default as Config } from "./Config"; | ||
export * from "./engine"; | ||
export type { default as Rule } from "./Rule"; |
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.
Should we export the other non-default types such as SchemaConfig
or Reporter
from the package root?
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 guess. I don't think anyone is creating custom reporters at this point, but if they want to then it would be useful. I don't think this is urgent though.
"$type : param of $param applies to table names and requires $expected", | ||
({ _type, param, expected1, expected2 }) => { | ||
|
||
const testCases = [ |
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.
All existing tables had the same contents, so I extracted them into one array. The duplication may have been intentional, but arrays require a lot of rows.
/** | ||
* The configuration for schemalint. | ||
*/ | ||
type Config = { |
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 chose the short name Config
. Would you prefer a more descriptive name, for example SchemalintConfig
?
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 think this is fine. It will likely only be used in configuration files where it will be clear what's being referenced.
Nice! Thank you! |
@kristiandupont Thank you for the merge! I would appreciate it if you could release a new version of the library to the npm registry so that we can use the newly exported types in JSDoc. |
Done. This also includes the bumped version of |
Fixes #338.