[MINOR][R] Fix indents of sparkR welcome message to be consistent with pyspark and spark-shell #23293

AzureQ · 2018-12-11T22:03:13Z

What changes were proposed in this pull request?

Removed empty space at the beginning of welcome message lines of sparkR to be consistent with welcome message of pyspark and spark-shell
Setting indent of logo message lines to 3 to be consistent with welcome message of pyspark and spark-shell

Output of pyspark:

Welcome to
      ____              __
     / __/__  ___ _____/ /__
    _\ \/ _ \/ _ `/ __/  '_/
   /__ / .__/\_,_/_/ /_/\_\   version 2.4.0
      /_/

Using Python version 3.6.6 (default, Jun 28 2018 11:07:29)
SparkSession available as 'spark'.

Output of spark-shell:

Spark session available as 'spark'.
Welcome to
      ____              __
     / __/__  ___ _____/ /__
    _\ \/ _ \/ _ `/ __/  '_/
   /___/ .__/\_,_/_/ /_/\_\   version 2.4.0
      /_/

Using Scala version 2.11.12 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_161)
Type in expressions to have them evaluated.
Type :help for more information.

How was this patch tested?

Before:
Output of sparkR:

 Welcome to
    ____              __
   / __/__  ___ _____/ /__
  _\ \/ _ \/ _ `/ __/  '_/
 /___/ .__/\_,_/_/ /_/\_\   version  2.4.0
    /_/


 SparkSession available as 'spark'.

After:

Welcome to
      ____              __
     / __/__  ___ _____/ /__
    _\ \/ _ \/ _ `/ __/  '_/
   /___/ .__/\_,_/_/ /_/\_\   version 2.4.0
      /_/


SparkSession available as 'spark'.

…rk work properly in in Docker container

rxin · 2018-12-12T00:17:33Z

R/pkg/inst/profile/shell.R

  cat("\n")

-  cat("\n SparkSession available as 'spark'.\n")
+  cat("\nSparkSession available as 'spark'.\n")


i thought this was here to align with the previous lines?

Yea,

Welcome to ____ __ / __/__ ___ _____/ /__ _\ \/ _ \/ _ `/ __/ '_/ /___/ .__/\_,_/_/ /_/\_\ version 3.0.0-SNAPSHOT /_/ - SparkSession available as 'spark'. + SparkSession available as 'spark'.

Maybe we can consider to check if all other shells messages look in the same format or coherent for some reasons, and then match them. But I wouldn't bother fixing it.

Yea, it's aligned with previous lines. Didn't see empty spaces in other shell cmd(spark-shell, pyspark). I'll close this if it's confirmed not necessary.

Can you show other shell messages and explain why this change makes it consistent 8n the PR description?

Updated the description.

HyukjinKwon · 2018-12-12T16:13:17Z

ok to test

HyukjinKwon · 2018-12-12T16:13:48Z

Let's update PR description too. Is it okay to you @rxin?

SparkQA · 2018-12-12T16:58:56Z

Test build #100034 has finished for PR 23293 at commit 988c9ca.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2018-12-13T02:05:51Z

R/pkg/inst/profile/shell.R

+  cat("   ____              __", "\n")
+  cat("  / __/__  ___ _____/ /__", "\n")
+  cat(" _\\ \\/ _ \\/ _ `/ __/  '_/", "\n")
+  cat("/___/ .__/\\_,_/_/ /_/\\_\\")


Don't these lines actually have to have 3 spaces in front to be consistent?

Right, I was thinking of getting rid of one empty space per line. But I just reset these lines back to origin.

Sorry I misunderstood it. Setting indent of these lines to 3 also.

HyukjinKwon · 2018-12-13T02:52:04Z

R/pkg/inst/profile/shell.R

+  cat("\nWelcome to")
  cat("\n")
  cat("    ____              __", "\n")
  cat("   / __/__  ___ _____/ /__", "\n")


I think @srowen meant we should have three spaces for this whole logo like:

____ __ / __/__ ___ _____/ /__ _\ \/ _ \/ _ `/ __/ '_/ /__ / .__/\_,_/_/ /_/\_\ /_/

Can you post the before/after results in the PR description at "How was this patch tested?"? Looks existing tests don't cover this change.

…nd spark-shell

HyukjinKwon · 2018-12-13T03:11:58Z

Looks fine. adding @felixcheung

SparkQA · 2018-12-13T03:31:46Z

Test build #100062 has finished for PR 23293 at commit 6deee33.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-13T03:40:59Z

R/pkg/inst/profile/shell.R

  if (nchar(sparkVer) == 0) {
    cat("\n")
  } else {
    cat("   version ", sparkVer, "\n")


Looks the space after version should be removed for consistency

Yep, just noticed that. Removed

SparkQA · 2018-12-13T03:49:55Z

Test build #100064 has finished for PR 23293 at commit ef42aba.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-12-13T04:34:05Z

Test build #100066 has finished for PR 23293 at commit fad25e2.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-13T04:55:39Z

Let's don't forget to update PR description and title as well. Both are kind of important in a way because that's going to be in commit messages. Sometimes it's easier to skim only commit messages to see what the commit changed. When both are mismatched, it causes potential overhead to skim what the commit changed.

AzureQ · 2018-12-13T05:09:03Z

Let's don't forget to update PR description and title as well. Both are kind of important in a way because that's going to be in commit messages. Sometimes it's easier to skim only commit messages to see what the commit changed. When both are mismatched, it causes potential overhead to skim what the commit changed.

Noted. Thanks a lot for the instructions. Super Helpful!

HyukjinKwon · 2018-12-13T05:22:44Z

@AzureQ, looks "How was this patch tested?" should be also updated. note "version 2.4.0" it has two spaces

HyukjinKwon · 2018-12-13T07:53:07Z

retest this please

SparkQA · 2018-12-13T08:05:01Z

Test build #100076 has finished for PR 23293 at commit fad25e2.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-13T08:17:35Z

retest this please

SparkQA · 2018-12-13T08:59:19Z

Test build #100079 has finished for PR 23293 at commit fad25e2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-12-13T12:04:08Z

Merged to master.

I fixed PR description by myself.

AzureQ · 2018-12-13T14:51:11Z

Merged to master.

I fixed PR description by myself.

Sorry for the delay. Thanks for the help!

…h pyspark and spark-shell ## What changes were proposed in this pull request? 1. Removed empty space at the beginning of welcome message lines of sparkR to be consistent with welcome message of `pyspark` and `spark-shell` 2. Setting indent of logo message lines to 3 to be consistent with welcome message of `pyspark` and `spark-shell` Output of `pyspark`: ``` Welcome to ____ __ / __/__ ___ _____/ /__ _\ \/ _ \/ _ `/ __/ '_/ /__ / .__/\_,_/_/ /_/\_\ version 2.4.0 /_/ Using Python version 3.6.6 (default, Jun 28 2018 11:07:29) SparkSession available as 'spark'. ``` Output of `spark-shell`: ``` Spark session available as 'spark'. Welcome to ____ __ / __/__ ___ _____/ /__ _\ \/ _ \/ _ `/ __/ '_/ /___/ .__/\_,_/_/ /_/\_\ version 2.4.0 /_/ Using Scala version 2.11.12 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_161) Type in expressions to have them evaluated. Type :help for more information. ``` ## How was this patch tested? Before: Output of `sparkR`: ``` Welcome to ____ __ / __/__ ___ _____/ /__ _\ \/ _ \/ _ `/ __/ '_/ /___/ .__/\_,_/_/ /_/\_\ version 2.4.0 /_/ SparkSession available as 'spark'. ``` After: ``` Welcome to ____ __ / __/__ ___ _____/ /__ _\ \/ _ \/ _ `/ __/ '_/ /___/ .__/\_,_/_/ /_/\_\ version 2.4.0 /_/ SparkSession available as 'spark'. ``` Closes apache#23293 from AzureQ/master. Authored-by: Qi Shao <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

AzureQ added 8 commits November 14, 2018 15:04

Copy python/pyspark to ${SPARK_HOME}/python/pyspark to make bin/pyspa…

c2f782b

…rk work properly in in Docker container

Added test for client mode pyspark shell into PythonTestsSuite

4bf6bc6

Correct pyspark shell test's driver image

c626bd1

Remove erroring test case for pyspark shell

149bed4

Merge remote-tracking branch 'upstream/master'

ff8d0ba

Updated docker-image-tool.sh with copying pyspark

5f6aaf8

Merge remote-tracking branch 'upstream/master'

3bf319f

Removed Empty Space at the beginning of SparkSession available msg

6631fad

rxin reviewed Dec 12, 2018

View reviewed changes

Removed empty spaces at begining of wecolme msgs

988c9ca

srowen reviewed Dec 13, 2018

View reviewed changes

reset logo msg lines indent

6deee33

HyukjinKwon reviewed Dec 13, 2018

View reviewed changes

Set indent of logo message lines to 3 to be consistent with pyspark a…

ef42aba

…nd spark-shell

HyukjinKwon reviewed Dec 13, 2018

View reviewed changes

Removed space after version

fad25e2

AzureQ changed the title ~~[MINOR][R] Removed Empty Space at the beginning of SparkSession available msg~~ [MINOR][R] Fixing indents of sparkR welcome message to be consistent with pyspark and spark-shell Dec 13, 2018

AzureQ changed the title ~~[MINOR][R] Fixing indents of sparkR welcome message to be consistent with pyspark and spark-shell~~ [MINOR][R] Fix indents of sparkR welcome message to be consistent with pyspark and spark-shell Dec 13, 2018

HyukjinKwon approved these changes Dec 13, 2018

View reviewed changes

asfgit closed this in 19b63c5 Dec 13, 2018

[MINOR][R] Fix indents of sparkR welcome message to be consistent with pyspark and spark-shell #23293

[MINOR][R] Fix indents of sparkR welcome message to be consistent with pyspark and spark-shell #23293

Uh oh!

Conversation

AzureQ commented Dec 11, 2018 • edited by HyukjinKwon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Dec 12, 2018

Uh oh!

HyukjinKwon commented Dec 12, 2018

Uh oh!

SparkQA commented Dec 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Dec 13, 2018

Uh oh!

SparkQA commented Dec 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 13, 2018

Uh oh!

SparkQA commented Dec 13, 2018

Uh oh!

HyukjinKwon commented Dec 13, 2018

Uh oh!

AzureQ commented Dec 13, 2018

Uh oh!

HyukjinKwon commented Dec 13, 2018

Uh oh!

HyukjinKwon commented Dec 13, 2018

Uh oh!

SparkQA commented Dec 13, 2018

Uh oh!

HyukjinKwon commented Dec 13, 2018

Uh oh!

SparkQA commented Dec 13, 2018

Uh oh!

HyukjinKwon commented Dec 13, 2018

Uh oh!

AzureQ commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AzureQ commented Dec 11, 2018 •

edited by HyukjinKwon

Loading

HyukjinKwon Dec 12, 2018 •

edited

Loading

HyukjinKwon Dec 13, 2018 •

edited

Loading