Skip to content

Conversation

@jaceklaskowski
Copy link
Contributor

@srowen
Copy link
Member

srowen commented Sep 17, 2015

This is a good fix, but this is another instance where I suspect the same issue exists in several markup files, like configuration.html. It's worth a JIRA since I think catching and fixing all of these is one non-trivial logical change.

If you can, avoid whitespace changes like stripping or adding space at the end of lines. It just adds to the diff and makes for a tiny extra chance of merge conflicts.

@jaceklaskowski
Copy link
Contributor Author

Is SPARK-10662 what you were thinking about? What should be the next steps? Guide me in the jira, please.

@srowen
Copy link
Member

srowen commented Sep 17, 2015

Perfect, yes, just modify the title of this PR to connect them. See https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark

@jaceklaskowski jaceklaskowski force-pushed the docs-yarn-formatting branch 2 times, most recently from dcb22b8 to b247811 Compare September 17, 2015 19:24
@jaceklaskowski jaceklaskowski changed the title [MINOR][DOCS] Fixes markup so code is properly formatted [SPARK-10662] [DOCS] Code snippets are not properly formatted in docs Sep 17, 2015
@jaceklaskowski jaceklaskowski changed the title [SPARK-10662] [DOCS] Code snippets are not properly formatted in docs [SPARK-10662][DOCS] Code snippets are not properly formatted in docs Sep 17, 2015
@srowen
Copy link
Member

srowen commented Sep 18, 2015

This much looks good other files need a similar treatment, at least the configuration doc.

@jaceklaskowski
Copy link
Contributor Author

So you want me to review the other documents for no-backtick-code-formatted-in-table issue? I'm fine with it, but just need to confirm my thinking.

@srowen
Copy link
Member

srowen commented Sep 18, 2015

Yeah I think that's worthwhile, as it should be fairly straightforward to locate instances of this -- look for the areas where tables are used and skim for backticks. At least, I already saw the configuration doc has this problem.

* Backticks should be <code>...</code> inside tables
* Few other minor formatting fixes
* Use YARN Application Master consistently in the YARN doc
@jaceklaskowski jaceklaskowski changed the title [SPARK-10662][DOCS] Code snippets are not properly formatted in docs [SPARK-10662][DOCS] Code snippets are not properly formatted in tables Sep 19, 2015
@jaceklaskowski
Copy link
Contributor Author

It should be better now. While fixing the docs, Atom Editor fixed the additional spaces at the end (that I remember you mentioned not to fix, but since it was done automatically and they are indeed an issue, please consider merging them, too). Thanks!

@srowen
Copy link
Member

srowen commented Sep 19, 2015

I think this is a good change. Yes, the whitespace changes are unfortunate, and would be better to use an editor that doesn't do that automatically.

@jaceklaskowski
Copy link
Contributor Author

Do you want me to split the pull requests to two - one with code formatting in table and another for the unfortunate excessive spaces? And what JIRA would that be - Removing excessive spaces in docs? I can do that, but want to be clear on your intents (that I however disagree with).

@srowen
Copy link
Member

srowen commented Sep 20, 2015

Your PR is making the whitespace changes which it ideally would not. I am not proposing whitespace changes; you are.

It wouldn't make sense to commit the changes and then go unchange it separately. At best, a new commit here would undo this. However I get that it is minor and tedious. This is more a request for future changes. I'm not sure what you disagree with?

@jaceklaskowski
Copy link
Contributor Author

I disagree with not accepting this change in this version with the superfluous spaces at the end of lines removed -- they're simply a garbage (and should not have been merged in the first place). They're quite likely leftovers from the time when it was decided to make the lines shorter.

Can you show me a line for which the space(s) is important? If there's one, I'll fix it right away.

@srowen
Copy link
Member

srowen commented Sep 20, 2015

The whitespace at the ends of the lines doesn't matter functionally either way and don't really need 'fixing'. The minor drawback to making such a change is spurious merge conflicts later, as I mentioned in #8795 (comment) These aren't worth it. If you want to make further docs changes, it would be best to not use an editor that would make these changes automatically. Still, probably best to just merge this, as it's unlikely to cause much if any trouble.

@jaceklaskowski
Copy link
Contributor Author

Still, probably best to just merge this, as it's unlikely to cause much if any trouble.

Would you? I'd greatly appreciate (and propose new changes :-))

@SparkQA
Copy link

SparkQA commented Sep 21, 2015

Test build #1780 has finished for PR 8795 at commit 4cc4087.

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

@srowen
Copy link
Member

srowen commented Sep 21, 2015

Merged to master

@asfgit asfgit closed this in ca9fe54 Sep 21, 2015
@jaceklaskowski jaceklaskowski deleted the docs-yarn-formatting branch September 21, 2015 21:20
Copy link
Contributor

Choose a reason for hiding this comment

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

@jaceklaskowski The <\code> end tag was wrong instead of <code>. Maybe it was a typo, but it will lead this page chaotic. I may PR to fix this later.
cc @srowen

ghost pushed a commit to dbtsai/spark that referenced this pull request Nov 14, 2015
`<\code>` end tag missing backslash in
docs/configuration.md{L308-L339}

ref apache#8795

Author: Kai Jiang <jiangkai@gmail.com>

Closes apache#9715 from vectorijk/minor-typo-docs.
asfgit pushed a commit that referenced this pull request Nov 14, 2015
`<\code>` end tag missing backslash in
docs/configuration.md{L308-L339}

ref #8795

Author: Kai Jiang <jiangkai@gmail.com>

Closes #9715 from vectorijk/minor-typo-docs.

(cherry picked from commit 9a73b33)
Signed-off-by: Sean Owen <sowen@cloudera.com>
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