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

Fix table caption RT #126740 #1291

Closed
wants to merge 9 commits into from
Closed

Fix table caption RT #126740 #1291

wants to merge 9 commits into from

Conversation

tbrowder
Copy link
Member

@tbrowder tbrowder commented Dec 1, 2017

appropriate tests are in a roast PR

@tbrowder
Copy link
Member Author

tbrowder commented Dec 2, 2017

The changes to Actions.nqp were required to get rid of a conflict—they have no part in my PR. It may be because my github fork was using the nom branch as default—I just changed it to master.

I tried to get the $.caption var set with a TWEAK submethod but couldn’t make it work.

@tbrowder
Copy link
Member Author

tbrowder commented Dec 2, 2017

Note the caption quote issue is another two bugs: RT #126742 and RT #130477. The quotes probably should be removed in Grammar.nqp, but could also be removed in Pod.nqp (in sub make_config).

Copy link
Contributor

@AlexDaniel AlexDaniel left a comment

Choose a reason for hiding this comment

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

No, that's not right. The change in src/Perl6/Actions.nqp shouldn't be there.

@AlexDaniel
Copy link
Contributor

What'd be the output of :bar<"meow">?

@tbrowder
Copy link
Member Author

tbrowder commented Dec 4, 2017

output of bar<“meow”>?

good question—I’ll add to tests and see...

output is: “meow”

surprising, but maybe not with various escaping of quotes in strings...

@AlexDaniel
Copy link
Contributor

I don't think I like it. So :bar<Q|meow|> will also give “meow”, it's mangling the input.

What would it take to make it not do that?

@tbrowder
Copy link
Member Author

tbrowder commented Dec 4, 2017

I don’t know. As Zoffix pointed out the scary TODO comments indicate some fragile handling—and I don’t yet understand how the value is actually extracted. I will play around with other nqp code to see how various things work...

I’m getting a glimmer of understanding, and I may be able to eventually determine when the <> is being used, but I don’t have enough time to work on this now. However, the quick fix as it is should take care of most issues since using something like :bar<“meow”> is a little unusual IMHO.

I think you should take my fix after I add a TODO to take care of those corner cases later.

@tbrowder
Copy link
Member Author

tbrowder commented Dec 4, 2017

TODO Update affected tickets to show incompleteness of fixes. Or, better, consolidate the tickets into one (and add number in code TODOs).

@AlexDaniel
Copy link
Contributor

AlexDaniel commented Dec 4, 2017

⚠⚠⚠ Rerevert Raku/roast#363 once this PR is merged (it was merged a bit too early by mistake, then reverted)

@tbrowder
Copy link
Member Author

tbrowder commented Dec 6, 2017

I’m closing this PR for now and will resubmit later in improved form.

@tbrowder tbrowder closed this Dec 6, 2017
@tbrowder tbrowder deleted the fix-table-caption-rt-126740 branch December 7, 2017 20:06
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.

None yet

3 participants