Skip to content

Conversation

@yu-iskw
Copy link
Contributor

@yu-iskw yu-iskw commented Jul 16, 2015

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 16, 2015

@shivaram Could you review it when you have time? Thanks!

@SparkQA
Copy link

SparkQA commented Jul 16, 2015

Test build #37486 has finished for PR 7439 at commit 61c391e.

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

Copy link
Member

Choose a reason for hiding this comment

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

Should we insert a white space into both sides of = ?
Could you fix this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. But personally, I think fixing it would be better with another issue. Since we haven't discussed the validation rule for the spaces for parameters yet.

@sarutak
Copy link
Member

sarutak commented Jul 16, 2015

I've left minor comments. Otherwise, LGTM.

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 16, 2015

@sarutak Thank you for reviewing it!

@shivaram
Copy link
Contributor

@yu-iskw FWIW we already have an (admittedly unwritten) style rule of putting space around both sides of =. We discussed this I think before SparkR was merged and its also a part of Hadley's style guide.
http://adv-r.had.co.nz/Style.html

BTW does lintr catch these or not ?

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 16, 2015

@shivaram Oh, sorry. I had fogotten.. Hmm, it seems that lintr couldn't catch these warnings. I'll check and get back to you.

Anyway, I'll fix them as @sarutak suggested.

@shivaram
Copy link
Contributor

Actually it comes up in a few more places but mostly in example code I guess
You can see this by running grep -r "[^ ]=[^ ]" $SPARK_HOME/R/pkg/R

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 16, 2015

@shivaram I have just experimented the infix operator warnings at function's optional parameters. According to the result, it seems that lintr can't catch them which are optional parameters.
https://gist.github.com/yu-iskw/be8398c17dc48f82c293

@yu-iskw
Copy link
Contributor Author

yu-iskw commented Jul 17, 2015

After all, I think it would be better to deal with the spaces problems on another issue. Because there is no linter about that, it is not easy to validate them now. Plus, we should focus on getting rid of the warnings about single-quotes strings on this PR.

@sarutak
Copy link
Member

sarutak commented Jul 17, 2015

Alright, let's focus on the single-quote string literals on this PR.
For now, I'm merging this into master and branch-1.4 and then, we can focus on another one.
@yu-iskw , thanks for your contribution!

@asfgit asfgit closed this in 5a3c1ad Jul 17, 2015
@sarutak
Copy link
Member

sarutak commented Jul 17, 2015

I got merge conflicts to branch-1.4. because #7395 is not merged into branch-1.4.
After #7395 is backported, we can easily backport this change too.
Feel free to open PRs for them.

@shivaram
Copy link
Contributor

Yeah backporting style improvements didn't seem very important to me. The only reason to backport them would be to avoid conflicts for other backport PRs -- Feel free to open a new JIRA / PRs if you think it'll make things easier in the future.

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.

4 participants