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

Strict null checks for coreutils #7607

Merged
merged 13 commits into from Dec 17, 2019
Merged

Conversation

vidartf
Copy link
Member

@vidartf vidartf commented Dec 10, 2019

References

Partially supersedes #7002.

Code changes

Some updated typings to be more clear about where null/undefined is expected/allowed, some bug fixes.

User-facing changes

None.

Backwards-incompatible changes

Strictly, these could all be categorized as bug fixes, but due to the number of minor tweaks, it might be better to do this as part of a major version release.

@afshin If this causes a major version release, it might be the timing for moving the settings registry etc, out of coreutils.

TODO before merging

  • Some bugs were left in place as I want input on how to resolve. These should be fixed before merging.
  • Individual questions below should all be resolved.
  • Each type in the public facing API in the package should be re-evaluated if it should also accept null/undefined... or we can leave it as is, and issue bug fixes if we need to expand the accepted values to include undefined/null. (this should be done at the end of all strict null PRs)

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Dec 10, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

Copy link
Member Author

@vidartf vidartf left a comment

@afshin There are multiple compile errors for the settings registry that I left in place. Would you mind having a look at those to figure out what the correct resolution is?

packages/coreutils/src/path.ts Outdated Show resolved Hide resolved
packages/coreutils/src/ratelimiter.ts Outdated Show resolved Hide resolved
packages/coreutils/src/ratelimiter.ts Outdated Show resolved Hide resolved
packages/coreutils/src/settingregistry.ts Show resolved Hide resolved
packages/coreutils/src/settingregistry.ts Show resolved Hide resolved
packages/coreutils/src/url.ts Show resolved Hide resolved
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 10, 2019

Strictly, these could all be categorized as bug fixes, but due to the number of minor tweaks, it might be better to do this as part of a major version release.

Great timing since the next release is a major one :)

@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 10, 2019

Great timing since the next release is a major one :)

It was never a guarantee that the coreutils package would get a major version bump though...

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 10, 2019

It was never a guarantee that the coreutils package would get a major version bump though...

We bumped all the packages this time around:

"version": "4.0.0-alpha.3",

@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 13, 2019

@afshin Do you have time to have a look at the current compile errors for restorablepool? I had a look at one way of resolving it, but wasn't sure of the nuances in the code.

@afshin
Copy link
Member

@afshin afshin commented Dec 13, 2019

@vidartf This should fix it:

--- a/packages/coreutils/src/restorablepool.ts
+++ b/packages/coreutils/src/restorablepool.ts
@@ -138,7 +138,7 @@ export class RestorablePool<

       if (objName) {
         const name = `${this.namespace}:${objName}`;
-        const data = this._restore.args(obj);
+        const data = this._restore.args?.(obj);

         Private.nameProperty.set(obj, name);
         await connector.save(name, { data });
@@ -298,7 +298,7 @@ export class RestorablePool<
     Private.nameProperty.set(obj, newName);

     if (newName) {
-      const data = this._restore.args(obj);
+      const data = this._restore.args?.(obj);
       await connector.save(newName, { data });
     }

@vidartf vidartf added this to the 2.0 milestone Dec 14, 2019
@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 14, 2019

Then this should be ready for review + merge and iterate for strict null checks for other packages.

afshin
afshin approved these changes Dec 14, 2019
Copy link
Member

@afshin afshin left a comment

Approved pending minor comments.

packages/coreutils/src/nbformat.ts Show resolved Hide resolved
packages/coreutils/src/statedb.ts Show resolved Hide resolved
@afshin
Copy link
Member

@afshin afshin commented Dec 17, 2019

Inertia wins the day on the great content vs. contents debate of 2019.

Thanks, @vidartf!

afshin
afshin approved these changes Dec 17, 2019
@afshin afshin merged commit 8b37871 into jupyterlab:master Dec 17, 2019
9 checks passed
@vidartf vidartf deleted the strict-coreutils branch Dec 18, 2019
@lock lock bot added the status:resolved-locked label Jan 17, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants