Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop extraneous spaces in code and table blocks #1307

Merged
merged 1 commit into from Dec 13, 2017

Conversation

softmoth
Copy link
Contributor

code2text() was missing a .join call, so inserted extra spaces between
chunks (e.g. around embedded formatting codes).

table2text() was padding the final column in each row, which is not
desirable in a plain text format.

@softmoth
Copy link
Contributor Author

There are no spec tests for Pod::To::Text. However, I have run this over all the pod in p6doc/doc, and verified that it looks good.

@AlexDaniel
Copy link
Contributor

AppVeyor failures are due to another issue (can be ignored).

👍 on the PR, looks good. But can we have some tests for the fixed issues? I think that's important because eventually POD stuff may be refactored, so it'd be sad to lose some of the fixes just because there are no tests.

@AlexDaniel AlexDaniel added the tests needed Issue is generally resolved but tests were not written yet label Dec 13, 2017
@softmoth
Copy link
Contributor Author

Alex, thank you for the review. I've added a new t/07-pod-to-text/01-whitespace.t file.

code2text() was missing a .join call, so inserted extra spaces between
chunks (e.g. around embedded formatting codes).

table2text() was padding the final column in each row, which is not
desirable in a plain text format.
@AlexDaniel
Copy link
Contributor

AlexDaniel commented Dec 13, 2017

I wonder though, what's the reason for these tests to not be in roast? Are these really implementation-specific?

@softmoth
Copy link
Contributor Author

I am pretty sure they don't belong in roast. Initially I put the file in spec/MISC, but it feels wrong to put in roast as the output isn't specified and to me it shouldn't be part of the language definition.

If that's not consensus then I'm okay putting it anywhere, just let me know where it belongs.

@AlexDaniel
Copy link
Contributor

We can move it later.

@AlexDaniel AlexDaniel merged commit 46eae46 into rakudo:master Dec 13, 2017
@softmoth softmoth deleted the pod-text-newlines branch December 13, 2017 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests needed Issue is generally resolved but tests were not written yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants