-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Re-introduce prematurely deprecated rdflib typings #42044
Re-introduce prematurely deprecated rdflib typings #42044
Conversation
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to rdflib with its source on master. Comparison details 📊
|
@Vinnl Thank you for submitting this PR! Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes. In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day! |
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!
context: linkeddata/rdflib.js#369
We've gotten sign-off from a reviewer 👏. A maintainer will soon review this PR and merge it if there are no issues. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for contributing to DefinitelyTyped! |
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.
remove from not needed types?
DefinitelyTyped/notNeededPackages.json
Line 3802 in 2eebcb4
"libraryName": "rdflib", |
@Vinnl One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you! |
2a85c4f
to
85c9cce
Compare
@jessetrinity Huh, good catch. Apparently it was moved around in a later commit, which is why the revert did not catch it. I manually removed it now. |
🔔 @jessetrinity - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
This reverts commit 974e381, introduced in #39502. That deprecated the type definitions, because a prerelease of rdflib (1.0.7-4) had inadvertently been tagged as
latest
. That's now been reverted, so the current latest version (again 1.0.6) no longer includes type definitions - this puts the DT ones back. Once 1.0.7 gets released, they can be removed again.Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If adding a new definition:
.d.ts
files generated via--declaration
dts-gen --dt
, not by basing it on an existing project.tslint.json
should be present and it shouldn't have any additional or disabling of rules. Just content as{ "extends": "dtslint/dt.json" }
. If for reason the some rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.tsconfig.json
should havenoImplicitAny
,noImplicitThis
,strictNullChecks
, andstrictFunctionTypes
set totrue
.