Skip to content

Conversation

@tkgdsg
Copy link

@tkgdsg tkgdsg commented Apr 16, 2017

You can paste GitBucket Gist Code in your website.

please review this patch.

You can paste GitBucket Gist Code in your website.
private def _gist(userName: String, repoName: Option[String] = None, revision: String = "master", isEmbed: Boolean = false): Any = {

if( repoName.isDefined && repoName.get.endsWith(".js")) {
return _gist(userName, Some(repoName.get.substring(0,repoName.get.length()-3)), revision, true)
Copy link
Member

Choose a reason for hiding this comment

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

Don't use return statement in Scala as much as possible because it's translated to throwing and catching an exception by the compiler.

Copy link
Author

Choose a reason for hiding this comment

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

I think I found better solution.
I changed function structure. Please review new one

files: Seq[(String, String)]
)(implicit context: gitbucket.core.controller.Context)
@import gitbucket.core.view.helpers
@import play.twirl.api.Html
Copy link
Member

Choose a reason for hiding this comment

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

It seems that play.twirl.api.Html isn't used in this template. So maybe this import statement is unnecessary.

Copy link
Author

Choose a reason for hiding this comment

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

I removed unnecessary import statement.

@prettifyCSS={<link href="@helpers.assets("/vendors/google-code-prettify/prettify.css")" rel="stylesheet">}
@gistCSS={<link href="@context.path/plugin-assets/gist/style.css" rel="stylesheet">}
@gitbucketCSS={<link href="@helpers.assets("/common/css/gitbucket.css")" rel="stylesheet">}
@adminLTECSS={<link href="@helpers.assets("/vendors/AdminLTE-2.3.8/css/AdminLTE.min.css")" rel="stylesheet">}
Copy link
Member

Choose a reason for hiding this comment

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

Why do you define these variables even through you are using them only once?

Copy link
Author

Choose a reason for hiding this comment

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

I modified that code to use strings directly in "document.write()".



private def _gist(userName: String, repoName: Option[String] = None, revision: String = "master"): Any = {
private def _gist(userName: String, repoName: Option[String] = None, revision: String = "master", isEmbed: Boolean = false): Any = {
Copy link
Member

Choose a reason for hiding this comment

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

Who pass true to isEmbed parameter?

Copy link
Author

Choose a reason for hiding this comment

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

I think I found better solution.
I changed function structure. Please review new one

@tkgdsg
Copy link
Author

tkgdsg commented Apr 19, 2017

I modified codes in response to the review.

@takezoe
Copy link
Member

takezoe commented Apr 20, 2017

@tkgdsg Looks good. Great thanks for this pull request!

@takezoe takezoe merged commit 2c51474 into gitbucket:master Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants