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

Remove UUID #4604

Merged
merged 8 commits into from Jul 4, 2018
Merged

Remove UUID #4604

merged 8 commits into from Jul 4, 2018

Conversation

@jirivrany
Copy link
Contributor

@jirivrany jirivrany commented May 18, 2018

Use phosphor's uuid function instead of uuid from coreutils. Closes #4579

Copy link
Contributor

@jasongrout jasongrout left a comment

Wow, we used that a lot of places! Two inline comments, and then it looks good to me.

Untitled.ipynb Outdated
@@ -0,0 +1,32 @@
{
Copy link
Contributor

@jasongrout jasongrout May 18, 2018

This file wasn't supposed to be committed, right? Can you either rebase the commit to not include it, or since it's small, it would also be fine to just add another commit deleting it.

@@ -13,4 +13,3 @@ export * from './statedb';
export * from './text';
export * from './time';
export * from './url';
Copy link
Contributor

@jasongrout jasongrout May 18, 2018

Can we still export a uuid function, which just wraps or is the phosphor function, for backwards compatibility?

Copy link
Contributor Author

@jirivrany jirivrany May 18, 2018

OK. Will do it.

Copy link
Contributor Author

@jirivrany jirivrany May 18, 2018

I kept the original file, but the content is now just a wrapper around UUID.uuid4 function.

s[i] = hexDigits.charAt(Math.floor(Math.random() * nChars));
}
return s.join('');
function uuid(): string {
Copy link
Contributor

@jasongrout jasongrout May 18, 2018

I didn't realize the old function had a length argument. Wow, that is definitely non-conforming.

We should probably keep the length argument, but it's not clear to me what we should do with it.

(a) We could ignore it (but that is basically changing the behavior, though perhaps not the worst sin in the world?).

(b) We could try to use it by generating the appropriate number of uuids and truncating the last one to be the right length.

(c) Or we could just bite the bullet, get rid of the function like you did originally, and declare that the next release is another major release.

I think (a) is a bad option since it breaks semver, and we've tried to be serious about semver in this repo. I think (b) is annoying, and question its usefulness. So I think I'm +0 on (c), and -0 on (b), and -1 on (a).

@ian-r-rose, @afshin?

Copy link
Contributor Author

@jirivrany jirivrany May 18, 2018

I can only say, that every usage of the function in the packages was using the default value. You can see in the changelog. But I don't know if the packages are used for extensions etc.

It may be possible to revert the changes in the packages, use the original uuid function as a wrapper around the UUID.uuid4() and truncate the result if the length is specified.

Copy link
Contributor

@jasongrout jasongrout May 18, 2018

It may be possible to revert the changes in the packages, use the original uuid function as a wrapper around the UUID.uuid4() and truncate the result if the length is specified.

I would say still move to using phosphor internally. The concern about backwards compatibility is because this is a package that a lot of outside extensions probably use too, and because we've already declared this package as 1.0 stability. Your proposal is basically what I mention in (b), though we'd have to handle lengths of more than 32 characters, so we'd first probably concatenate together the appropriate uuid strings

Copy link
Contributor Author

@jirivrany jirivrany May 18, 2018

OK. I will wait for the further instructions. Meanwhile I will look around for another good-first-issue where I can help :)

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 18, 2018

@jasongrout jasongrout added this to the Beta 3 milestone Jun 6, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 28, 2018

Unfortunately, we waited too long to give you feedback on this, so now there are merge conflicts. @jirivrany - would you like to cherry-pick your commit on top of master and fix the conflicts?

I think the decision is to: (a) move all uses of uuid() to use phosphor's uuid, and (b) add a note in the documentation for the jlab uuid that the function is deprecated and will be removed in the future.

@jirivrany
Copy link
Contributor Author

@jirivrany jirivrany commented Jun 28, 2018

@jasongrout sure I will do it. Should I rebase with current master, or start new PR from scratch? What do you think?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 28, 2018

Should I rebase with current master, or start new PR from scratch? What do you think?

A rebase might work out. What I did to test was cherry-pick just your 0141655 commit onto master, which had just a few places that needed fixing. You could force-push the result to here to get rid of the commits above, or start a new PR, whichever you find easier.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 28, 2018

Thank you!

Let's leave the existing uuid() function exactly as it is for backwards compatibility, and leave the test file as well, and only put a note in the documentation saying it is deprecated and please switch to the phosphor function.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 28, 2018

It looks like the test failures are real things that should be fixed as well (integrity notes that some files don't end in newlines anymore, etc.)

@jirivrany
Copy link
Contributor Author

@jirivrany jirivrany commented Jun 28, 2018

@jasongrout ok, i will revert the changes of the function. Just for clarification - by documentation you mean the doc-string in the code or another place?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 28, 2018

Thanks. Can you put back the test file too? And can we tweak the sentence in the docstring to end with 'use ... instead.' (remove the of and end with a period).

Thanks!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 29, 2018

Looks like one linting error: ERROR: packages/services/test/src/session/session.spec.ts[657, 4]: file should end with a newline

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 4, 2018

To update to the prettier changes, I squashed the commits here and used precise-commit and a merge process inspired by #4330 (comment). I also removed some of the whitespace changes that caused unnecessary conflicts with prettier.

jirivrany and others added 3 commits Jul 4, 2018
deleted @jupyterlab/coreutils/uuid.ts

change coreutils/uuid.ts to wrapper of phosphor/coreutils/UUID.uuid4

replaced @jupyterlab/coreutils/uuid.ts with @phosphor/coreutils/uuid.ts

deleted @jupyterlab/coreutils/uuid.ts

remove unused dependencies @jupyterlab/coreutils

keep original uuid but mark it as deprecated in docs

return back test for uuid

resolve conflict in session.spec.ts
jasongrout added 2 commits Jul 4, 2018
This also deletes a few duplicate imports that made it through the merge.
@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jul 4, 2018

This looks good to me. Should the juptyterlab version be deprecated somehow? I didn't see that in the changes.

@jirivrany
Copy link
Contributor Author

@jirivrany jirivrany commented Jul 4, 2018

@saulshanabrook it is marked as deprecated in the doc string https://github.com/jupyterlab/jupyterlab/pull/4604/files#diff-5f0044d566071c32a20cb7bcbca3b03d I'm still relatively new in TypeScript, so I'm not sure if there is something like Java deprecated notation or so?

jasongrout added 2 commits Jul 4, 2018
After more discussion, we decided since we have a major version bump in coreutils, we should just remove the uuid function after all.
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 4, 2018

@jirivrany - sorry for all the back and forth and changing our minds about what we wanted. Thanks for all of your work on this PR!

afshin
afshin approved these changes Jul 4, 2018
Copy link
Member

@afshin afshin left a comment

Thanks @jirivrany and @jasongrout!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 4, 2018

The notebook does not accept - in config section names: https://github.com/jupyter/notebook/blob/b2edf8963cc017733f264cca35fd6584f328c8b6/notebook/services/config/handlers.py#L36

So I'll change the tests.

@jirivrany
Copy link
Contributor Author

@jirivrany jirivrany commented Jul 4, 2018

No problem @jasongrout I'm happy that I can help and learn something new.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 4, 2018

Approved and passing. Merging.

@jasongrout jasongrout merged commit ab95c4e into jupyterlab:master Jul 4, 2018
2 checks passed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants