Fix for "Footnote titles not translatable #244" #247

davidlowryduda · 2017-02-27T23:33:46Z

Resolves #244

There are a few aesthetic options about how one might try to allow different language notes in footnotes in markdown. I find it a bit hard to allow this in a nice way from direct command line usage, so I've chosen an implementation through in-interpreter use.

The default footnote title is "Jump back to footnote %d in the text." To change, I propose the following.

markdowner = markdown2.Markdown(extras=["footnotes"])
markdowner.footnote_title = "Retournez vers footnote %d. "
markdowner.convert(text)

The extra information is passed as an attribute of the markdown2.Markdown class used for conversion. If there is a format problem, then I revert to the default.

Note: I'm having trouble running tests on my own [I get 22 failures on the current master branch], so I expect to need to clean up a few things first.

As noted in Issue trentm#234 trentm#234 the metadata extra uses regex that is too greedy and can eat more than what is intended if links appear in the markdown. This small change corrects this. Resolves: trentm#234

The previous commit towards resolving Issue trentm#234 didn't incorporate previous behavior that fences shouldn't be necessary. First introduced in Pull Request trentm#215, metadata should be accessible even without including `---` fences around opening metadata. This fix now preserves the ability to use either `---` fences (maybe without a subsequent blank line) or no fences (but with a blank line). Resolves: trentm#234

As noted in Issue trentm#244, it is not currently possible to customize the text used in footnotes. It is always "Jump back to footnote <number> in the text." It may be desirable to provide a programmable way to supply different text, such as in different languages. This provides that support when used from within the interpreter. Usage is markdowner = markdown2.Markdown(extras=["footnotes"]) markdowner.footnote_title = "Retournez vers footnote %d. " markdowner.convert(text) This also adds the ability to change the footnote link text, through markdowner.footnote_link_text = "<something>" because they are very similar. Resolves: trentm#244

davidlowryduda · 2017-02-28T00:19:03Z

Good, after fixing a silly mistake, all tests pass.

In principle it might be a good idea to write a test for a custom footnote string. However I'm afraid that since this is designed to include a class variable, I don't know how to incorporate such a test into the current test suite.

nicholasserra · 2017-02-28T05:07:41Z

lib/markdown2.py

+
+            try:
+                footnote_title = self.footnote_title
+            except Exception:


Can we change these naked except Exception: to except AttributeError:

nicholasserra · 2017-02-28T05:10:22Z

lib/markdown2.py

+            try:
+                footnote_title = self.footnote_title
+            except Exception:
+                footnote_title = 'title="Jump back to footnote %d in the text.">'


So is it implied here that they need to include the closing > in their custom title? And on line 2049 they need to include the closing anchor tag? Feels kinda dirty. Id rather see these inputs be exactly what they're described as: the title and link text. The DOM pieces should not be required and should remain in the backlink text.

You're absolutely right. I'll change this.

nicholasserra · 2017-02-28T05:10:45Z

lib/markdown2.py

+                            'class="footnoteBackLink" ' +
+                            footnote_title +
+                            footnote_link_text) % (id, i+1)
+                except Exception as E:


What Exception are you expecting to see here? Can we be specific about what is caught?

I'm expecting a TypeError if a user doesn't include a "%d" in their footnote_title. I've now made this explicit.

nicholasserra · 2017-02-28T05:11:29Z

Thanks! Had some implementation questions and some python specific nitpicks on this. But I like the direction and think we can make this work.

Users no longer need to include html closing tags in custom definitions for footnote_title or footnote_link_text. And naked except Exception statements have been replaced with the expected errors. These notes were made by nicholasserra in Pull Request trentm#247

davidlowryduda · 2017-02-28T16:17:45Z

Thank you nicholasserra very much for your detailed comments. I appreciate you taking the time give me feedback.

nicholasserra · 2017-02-28T20:18:11Z

lib/markdown2.py

+            try:
+                footnote_title = self.footnote_title
+            except AttributeError:
+                footnote_title = 'title="Jump back to footnote %d in the text."'


Should we make this even more specific, thinking title being the actual title text vs having the end user make sure to include title=""? Isn't the title arg always going to be required?

My thought would be footnote_title = 'Jump back to footnote %d in the text.'

Yes, you're right. Good eye.

nicholasserra · 2017-02-28T20:19:06Z

Some more minor feedback. This is looking great. Can we add some tests too?

davidlowryduda · 2017-02-28T23:34:02Z

@nicholasserra I would be happy to add some tests, but I'm afraid I might need a little guidance on the best way to proceed.

Right now, the tests are all done by calling markdown2.markdown(text, **opts) directly. However, in my proposed setup for providing custom footnote languages, it is necessary to insert a class variable in the middle. That is, one would want something like

markdowner = markdown2.Markdown(**opts)
markdowner.footnote_title = "Custom Title, like see %d"
markdowner.convert(text)

How would you recommend adding tests for the desired behavior? Of course, it would be possible to write some sort of one-off test, but I suspect that there is a better way.

davidlowryduda · 2017-03-01T01:51:41Z

As I think more about the tests, I believe I can draw inspiration from the tests for the metadata extra.

nicholasserra · 2017-03-01T16:13:44Z

Cool. Give it a shot. Let me know if you hit a dead end 👍

It is now possible to provide class variables to the Markdown instance in the testing library by creating a file.customargs file containing a dictionary. For example, one might have have a footnotes_customtitle test, which could be tested by making the following file. footnotes_customtitle.customargs --- {"footnote_title" : "Return to %d above"} --- When testing footnote_customtitle, the Markdown instance will be passed the variable, essentially like ``` markdowner = markdown2.Markdown(*opts) markdowner.footnote_title = "Return to %d above" text = markdowner.convert(html) ``` This was done to allow tests for trentm#247

There is now a test for custom footnote_titles in the test suite. Adds tests for: PR trentm#247 Resolves: trentm#244

davidlowryduda · 2017-03-04T03:16:51Z

Ok, I've now added a test. At the moment, I've only added a single test (footnotes_custom). It's not hard to add more, but this seemed fine to me.

Earlier, I'd been a bit concerned about the right way to add this test functionality. I ended up allowing a new type of file in the test directories, named like footnotes_custom.customargs. I don't actually like the name .customargs but I couldn't think of a better one immediately. Perhaps .custs (for customs) or .classvars (as these are class variables for the Markdown class)?

If additional functionality happens to be provided through class variables in the future, then this now fits quite easily into the testing framework.

nicholasserra · 2017-03-06T05:16:29Z

Nice. I'll give this a final look tomorrow. After staring at this for a while i'm debating the effect of using this format as proposed:

markdowner = markdown2.Markdown(extras=["footnotes"])
markdowner.footnote_title = "Retournez vers footnote %d. "
markdowner.convert(text)

vs something like

markdowner = markdown2.Markdown(extras=["footnotes"], footnote_title = "Retournez vers footnote %d. ")
markdowner.convert(text)

Apologies for this line of thought so late in the game. But trying to keep in mind any more future requests like this. And now just remembering that the link-patterns extra is already doing something similar, passing:

markdown2.markdown('test.' extras=["link-patterns"], link_patterns=link_patterns)

... directly into the markdown() method.

Thoughts?

davidlowryduda · 2017-03-06T07:51:40Z

Yes, it's certainly true that this introduces a different format from other extras or customizations.

I think one good aspect of this approach is that it's quite straightforward if future requests were to also use class variables, and my changes to the testing suite allow a uniform approach to writing tests.

However, I think an undesirable aspect is that this is not callable directly from the commandline (unlike every other part of python-markdown2). It's easy enough to script away, but perhaps this is a sign.

I hadn't paid attention to the link-patterns extra, but you're right that the link-patterns extra accomplishes a similar goal by having another kwarg hanging around. I now see that self.link_patterns is immediately defined as link_patterns --- so there is also a class variable declaration there. I like that this is callable directly from the commandline (maybe with a link-patterns-file). I dislike the idea that a few additional extras might lead to additional kwargs and additional config files. [But this is not currently a problem].

Anyhow, if you think it would be better if the functionality worked like

markdown2.markdown(text, extras=["footnotes"], footnote_title="Something %d.")

then that is something I'm content to rewrite and make happen. But I wonder, what would we really want? For example, a related look might be

markdown2.markdown(text, extras=["footnotes"], extras-config={footnote_title: "Something %d."}

or perhaps

markdown2.markdown(text, extras=["footnotes"], extras-config={
    "footnotes":  {
        "footnote_title": "Something %d."
    }
})

where now extra things are passed in as key-value pairs in a dictionary of configurations. If one wants to change both the footnote_title (the actual text) and the footnote_title_link (by default, the unicode arrow U+21A9, ↩), then one could do

markdown2.markdown(text, extras=["footnotes"], extras-config={
    "footnotes":  {
        "footnote_title": "Something %d.",
        "footnote_title_link": "&#8617;"
    }
})

This is an option. Is it a better option? Maybe. I think I like it more than having the kwargs footnote_title and footnote_title_link around in the markdown2.markdown(...) method.

nicholasserra · 2017-03-06T17:26:11Z

Thanks for the thoughts. I think I would like it more as

markdown2.markdown(text, extras=["footnotes"], footnote_title="Something %d.")

Now that I see that pattern already being used with other extras. But I totally agree that something like

markdown2.markdown(text, extras=["footnotes"], extras-config={footnote_title: "Something %d."}

... seems cleaner. BUT I don't want to mess with backwards compatibility.

I think we should go with that markdown2.markdown(text, extras=["footnotes"], footnote_title="Something %d.") option for now, to be consistent.

And maybe sometime in the future, if we see more extras requiring attribute input, we can refactor into some kind of extras-config attribute and bump to a backwards incompatible version.

Thanks again and sorry for these pivots.

The behavior of custom footnote text is now implemented in the following way, as recommended in trentm#247 ``` markdown2.markdown(text, extras=["footnotes"], footnote_title="Regresar a %d.") ``` This is in contrast to the previous implementation, which required an intermediate step defining a class variable. As the interface changed, it was necessary to rewrite the relevant test, which now defines the customization in the footnotes_custom.opts file.

davidlowryduda · 2017-03-07T07:36:04Z

I understand, and now I believe it works as (now) intended. I updated the test as well, and removed the custom class-variable component.

nicholasserra · 2017-03-07T18:35:12Z

Thank you for all of the work on this, much appreciated!

davidlowryduda · 2017-03-07T20:58:31Z

And thank you for the feedback.

davidlowryduda added 7 commits December 18, 2016 15:04

Fix metadata extra to consume only initial fenced block

7ef8247

As noted in Issue trentm#234 trentm#234 the metadata extra uses regex that is too greedy and can eat more than what is intended if links appear in the markdown. This small change corrects this. Resolves: trentm#234

Add David Lowry-Duda as a contributor

85dc8d9

Resolved merge conflict on CONTRIBUTORS

b1e79df

using upstream changes to CONTRIBUTORS

3424108

Added footenoteBackLink class to footnotes

3fcf2a1

nicholasserra reviewed Feb 28, 2017

View reviewed changes

"title=" no longer required for custom footnote title

45a3139

davidlowryduda added 3 commits March 3, 2017 18:32

Added missing quotes to footnote_titles

ff0d20c

Added test for custom footnote_title to test suite

9ef14e8

There is now a test for custom footnote_titles in the test suite. Adds tests for: PR trentm#247 Resolves: trentm#244

nicholasserra merged commit 38ee311 into trentm:master Mar 7, 2017

Fix for "Footnote titles not translatable #244" #247

Fix for "Footnote titles not translatable #244" #247

Uh oh!

Conversation

davidlowryduda commented Feb 27, 2017

Uh oh!

davidlowryduda commented Feb 28, 2017

Uh oh!

nicholasserra Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

nicholasserra Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

davidlowryduda Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

nicholasserra Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

davidlowryduda Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

nicholasserra commented Feb 28, 2017

Uh oh!

davidlowryduda commented Feb 28, 2017

Uh oh!

nicholasserra Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

davidlowryduda Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

nicholasserra commented Feb 28, 2017

Uh oh!

davidlowryduda commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidlowryduda commented Mar 1, 2017

Uh oh!

nicholasserra commented Mar 1, 2017

Uh oh!

davidlowryduda commented Mar 4, 2017

Uh oh!

nicholasserra commented Mar 6, 2017

Uh oh!

davidlowryduda commented Mar 6, 2017

Uh oh!

nicholasserra commented Mar 6, 2017

Uh oh!

davidlowryduda commented Mar 7, 2017

Uh oh!

nicholasserra commented Mar 7, 2017

Uh oh!

davidlowryduda commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davidlowryduda commented Feb 28, 2017 •

edited

Loading