Skip to content

Conversation

@astroshim
Copy link
Contributor

What is this PR for?

It's better if notebook name can be normalized.

What type of PR is it?

Improvement

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-957

How should this be tested?

Try create notebook name with many slashes and no slashes.
You can refer to screen shot.

Screenshots (if appropriate)

  • before
    be
  • after
    af

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@cloverhearts
Copy link
Member

LGTM

}
private String normalizeNoteName(String path){
path = path.trim();
path = path.replace("\\", "/");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this change windows path like mynotebooks\notebook1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, That line change for the windows path to '/'.

@corneadoug
Copy link
Contributor

Tested

Merging if there is no more discussions

@felixcheung
Copy link
Member

@astroshim
Copy link
Contributor Author

@felixcheung Thank you for pointing out that i missed. I just fixed it.

@Leemoonsoo
Copy link
Member

Is normalization applied on note name change?

@Leemoonsoo
Copy link
Member

Leemoonsoo commented Jun 11, 2016

Thanks @astroshim for the improvement. I've tested and although it's working well, i think there're little more things we can think

  1. Normalization is not applied when change note name.
  2. Normalization routine need to be placed in zeppelin-zengine, rather than zeppelin-server
    zeppelin-server is place for the code that enables front-end / back-end communication.
    business logic handles notebook should be in zeppelin-zengine module.
  3. It would be great to have a unittest for normalize note name
  4. Note name display is bit inconsistent.

with this branch

Given name when create note note name displayed on screen
MyNote MyNote
/MyNote /MyNote
MyNote/sub /MyNote/sub
/MyNote/sub /MyNote/sub

but i think it should be like

Given name when create note note name displayed on screen
MyNote MyNote
/MyNote /MyNote
MyNote/sub MyNote/sub
/MyNote/sub /MyNote/sub

@astroshim
Copy link
Contributor Author

I really appreciate your detailed review. @Leemoonsoo
I will fix 1,2,3 but in case 4,
I thought /MyNote/sub and MyNote/sub are the same from directory point of view.
Do you think they should be treated differently?

@Leemoonsoo
Copy link
Member

@astroshim Thanks!
MyNote and /MyNote are treated differently. And i think it's right. (compatible to previous version)
Therefore I think it's more consistent to treat differently MyNote/sub and /MyNote/sub, too.

@astroshim
Copy link
Contributor Author

@Leemoonsoo Thank you for kindly explaination. I understand about name.

@astroshim
Copy link
Contributor Author

@Leemoonsoo I fixed 1,2,3,4. please review.

@Leemoonsoo
Copy link
Member

Thanks for quick response. LGTM

@Leemoonsoo
Copy link
Member

Merge it into master if there're no more discussions

@asfgit asfgit closed this in bda1cc5 Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants