Convert FEED setting from `%s` to `{slug}`-style strings #2383

MinchinWeb · 2018-07-12T05:34:06Z

justinmayer · 2018-07-12T09:33:34Z

@MinchinWeb: It appears Travis is reporting test failures. Could you take a look?

MinchinWeb · 2018-07-13T00:32:06Z

Flake8 issues have been fixed.

I have Pandoc v2.1.3 install locally and so some test fail due to that. See issues #2255 and #2322, and WIP PR #2289.

There are several other tests that fail locally (on Windows) for various reasons; some are things I haven't touched, so I'm leaving alone; some are due to 'symbolic link privilege not help'; some are due to Windows filepaths being written different than POSIX paths (these I skip now on Windows). These, as a whole, I'm ignoring.

The other broken test was a simple typo. Fixed now!

Python 3.7 has been released, but was failing to download on Travis; it can be added back at a later time.

justinmayer · 2018-08-03T13:19:10Z

Would any @getpelican/reviewers be willing to take a moment and review this pull request?

mosra · 2018-09-12T11:57:22Z

Yay, I personally welcome the change to {}-style! (Was also wondering myself why feeds use %s while everything else is {}-style.) But I think this should be done in a backwards-compatible way. With your proposed change, all setting that had the old-style

CATEGORY_FEED_ATOM = 'feeds/%s.atom.xml'

would now result in feeds/%s.atom.xml put verbatim into the generated output, without any warning that something's wrong. Discovering such breakage in each affected site can then take months and that's bad. So instead just calling format(), I'd propose something like this:

self.settings['CATEGORY_FEED_ATOM']).format(slug=category.slug).replace('%s', category.slug)

And similarly elsewhere (maybe replace() first? not sure). That'll work for both approaches, with the replace() being removed some years later once the backwards compatibility is not needed anymore.

Besides that, the diff contains a lot of unnecessary formatting changes that make it very hard to review. Would it be possible to clean up the diff to not have them? Also, unrelated changes such as posix_path() or Windows test fixes could be better discussed in a separate PR.

MinchinWeb · 2018-09-18T03:40:47Z

@mosra : I like you suggestion of how to support the two styles. I searched for examples of how to do this, and never did find anything useful...

Regarding the "extra" pieces, some of the reformatting is from the different lengths of {slug} vs %s, some was for line-length consistency. The second group could be removed, but there is no automated way to do it, I'd basically have to completely rebuilding the patch. The posix_path() changes again were for consistency. If you really want these two pieces out, I could maybe pull them out to a new PR, and then rebase this one on top of it. Is is worth the extra effort?

The Windows testing issues are similar (they could be pulled to a separate PR, and this one rebased on to it), but they were needed to get this far: when I first ran the test suite locally (on my Windows machine), a huge swatch of the tests failed, and so it was very hard to know what, if any, tests I had broken. Making the tests behave more sanely was the faster alternative to spamming Travis and crossing my fingers.

MinchinWeb · 2018-09-18T03:59:57Z

@justinmayer @mosra : both old-style and new-style text substitutes are now supported!

self.settings['CATEGORY_FEED_ATOM'].replace('%s', '{slug}')
                                   .format(slug=cat.slug),

mosra · 2018-09-19T14:12:16Z

pelican/generators.py

@@ -400,7 +402,8 @@ def generate_feeds(self, writer):
                    writer.write_feed(
                        arts,
                        self.context,
-                        self.settings['TAG_FEED_ATOM'].format(tag.slug),
+                        self.settings['TAG_FEED_ATOM'].replace('%s', '{slug}')
+                                                      .format(tag.slug),


Why not .format(slug=tag.slug) here (and the two cases below)?

Good catch. It's been fixed!

avaris · 2018-10-02T19:57:47Z

Sorry for late-ish response but, I don't really like the %s replacement.

For one, it'd fail if you have {}s around %s:

>>> '%s'.replace('%s', '{slug}').format(slug='foo')
'foo'
>>> '{%s}'.replace('%s', '{slug}').format(slug='foo')
'{slug}'
>>> '{%s'.replace('%s', '{slug}').format(slug='foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Single '}' encountered in format string

Second, I'd rather have these handled when the settings file is read (in settings.py).

And I'd propose throwing a warning and falling back to the default in case %s is in these settings.

MinchinWeb · 2018-10-04T18:05:43Z

@avaris I like the idea of moving the conversion to the settings.py file.

I'm not sure what to do about the errors you mention, and I haven't found an example online support both string styles at the same time. Do we throw an error on %s strings, and make the users upgrade? Do we throw a warning, try the conversion, and hope for the best?

avaris · 2018-10-04T18:47:16Z

A check in settings.py. If '%s' is in the string, then throw a warning about deprecation, replace it from the DEFAULT_CONFIG. Something like:

for KEY in list_of_feed_setting_keys:
    if '%s' in setting[KEY]:
        logger.warning('%%s usage in %s is deprecated, use {name} instead. Falling back to default'. KEY)
        setting[KEY] = DEFAULT_CONFIG[KEY]

Very similar to this.

rather than trying to update them in place. Relies on the user to make the needed changes. Fallback is the default setting.

MinchinWeb · 2018-10-13T16:10:58Z

@avaris @mosra : I've moved to avaris' suggestion of complaining of old style settings when the settings are first loaded and falling back to the default in this case. I'm of slightly mixed feeling as it requires the user to actively update their settings to work with the new Pelican version, and doesn't support backwards-compatibility, but on the other hand it avoids a bunch of odd corner cases that are hard to detect and resolve automatically. But "explicit is better than implicit", so let's go with it.

Does something in the documentation need to be added to explain this change?

The failing test is due to a change in how Markdown v3 outputs HTML for footnotes. The fix has been submitted separately as PR #2417.

justinmayer · 2018-10-23T16:31:03Z

@avaris / @mosra: Any further comments on @MinchinWeb's pull request?

avaris · 2018-10-30T15:22:20Z

Stuff related to this PR is OK, but there are a lot of unrelated stuff (some unwarranted formatting changes, tox?? etc) attached to it. Is it possible to clean this up a bit?

justinmayer · 2018-11-01T09:29:26Z

@MinchinWeb: I agree it's best to keep pull requests as discrete and digestible as possible. Could you take a look and perhaps move anything not directly related to issue 2106 into separate pull requests?

MinchinWeb · 2018-11-02T21:26:47Z

It can be done, but it does seem like "busy work" rather than "fun work". And I don't understand what it adds.

avaris · 2018-11-02T21:42:46Z

It's more about what it removes, which is the unrelated stuff reviewers have to go over otherwise.

Closes getpelican#2106, Closes getpelican#2383.

MinchinWeb · 2018-11-03T03:39:24Z

7 Pull Requests and 5 hours later, the pieces of this have all been split apart. #2432 is the core functionality I set out to add. I hope the various pull requests can be merged quickly and cleanly.

Travis: Add Windows testing support #2428 -- add Windows and Python 3.7 support to Travis-CI
Docs: Reflow to 79 char line limit #2429 -- reflow text to 79 character line limit
Settings: build all paths with posix_join #2431 -- build all paths using posix_join()
Convert FEED settings from %s to {slug} redux #2432 -- covert all FEED settings from %s to {slug} (or {lang})

* Convert FEED settings from `%s` to `{slug}` redux Closes #2106, Closes #2383

Since Pelican 4.0, `CATEGORY_FEED_ATOM` and `TAG_FEED_ATOM` are expected to use `{slug}`, not `%s`. Seems to have been introduced in getpelican/pelican#2383. This would cause the following error: ``` CRITICAL: TypeError: not all arguments converted during string formatting ``` This change allows for backwards and forward compatibility.

Since Pelican 4.0, `CATEGORY_FEED_ATOM` and `TAG_FEED_ATOM` are expected to use `{slug}`, not `%s`. Seems to have been introduced in getpelican/pelican#2383. This would cause the following error: ``` CRITICAL: TypeError: not all arguments converted during string formatting ``` This change allows for backwards and forward compatibility. Cherry picked from commit getpelican/pelican-themes@be0a410.

…lican#2431. c.f. getpelican#2383.

Convert RSS setting from %s to {slug}-style strings

06f637a

MinchinWeb mentioned this pull request Jul 12, 2018

TAG_FEED_ATOM = 'label.{slug}.atom.xml' breaks Pelican #2106

Closed

Use posix_join() through settings

31c8ac6

MinchinWeb added 4 commits July 12, 2018 16:30

Tox: add Python 3.7

0dc7e65

Fix flake8 issues

9e89a95

skip tests on Windows that require POSIX paths

9bf6a81

Fix broken test

1f8087c

MinchinWeb force-pushed the feed-slugs branch from 0368fc2 to 1f8087c Compare July 13, 2018 00:11

Remove Python 3.7 from Tox

a19d8f9

MinchinWeb force-pushed the feed-slugs branch from 257b803 to a19d8f9 Compare July 13, 2018 00:26

Support both old and new style string substitutions

6e4a26e

justinmayer requested a review from mosra September 18, 2018 09:13

mosra reviewed Sep 19, 2018

View reviewed changes

MinchinWeb force-pushed the feed-slugs branch from 91c9018 to 88fa66d Compare September 20, 2018 04:28

Be consistent in .format() usage

5be4b17

MinchinWeb force-pushed the feed-slugs branch from 88fa66d to 5be4b17 Compare September 20, 2018 04:30

MinchinWeb force-pushed the feed-slugs branch 2 times, most recently from 39e2820 to fba4c84 Compare October 13, 2018 15:40

Warn on old format settings

a3c0360

rather than trying to update them in place. Relies on the user to make the needed changes. Fallback is the default setting.

MinchinWeb force-pushed the feed-slugs branch from fba4c84 to a3c0360 Compare October 13, 2018 15:40

justinmayer requested a review from avaris October 23, 2018 16:31

justinmayer added the awaiting review label Oct 24, 2018

avaris closed this Oct 30, 2018

avaris reopened this Oct 30, 2018

justinmayer added needs more work and removed awaiting review labels Nov 1, 2018

This was referenced Nov 3, 2018

Travis: Add Windows testing support #2428

Closed

Docs: Reflow to 79 char line limit #2429

Merged

Settings: build all paths with posix_join #2431

Closed

Fix DeprecationWarning in tests about collections.abc #2389

Merged

MinchinWeb added a commit to MinchinWeb/pelican that referenced this pull request Nov 3, 2018

Convert FEED settings from %s to {slug} redux

cceb04d

Closes getpelican#2106, Closes getpelican#2383.

This was referenced Nov 3, 2018

Convert FEED settings from %s to {slug} redux #2432

Merged

Add Python 3.7 support to Travis and Tox #2427

Merged

justinmayer closed this in #2432 Nov 3, 2018

justinmayer pushed a commit that referenced this pull request Nov 3, 2018

Convert FEED settings from %s to {slug} redux (#2432)

3a0add4

* Convert FEED settings from `%s` to `{slug}` redux Closes #2106, Closes #2383

MinchinWeb mentioned this pull request Nov 3, 2018

Fix Indent on Switch-to-{slug} Warning Code #2433

Merged

avaris mentioned this pull request Nov 9, 2018

Replace %s rather than fall back to defaults #2437

Merged

MinchinWeb deleted the feed-slugs branch November 22, 2018 05:12

bryanbrattlof mentioned this pull request Dec 7, 2018

Determine where TypeError: not all arguments converted during string formatting is occuring? #2489

Closed

pedrohdz mentioned this pull request Jan 20, 2019

pelican-bootstrap3: Handle Pelican >= 4.0 slugs in feeds getpelican/pelican-themes#633

Merged

MinchinWeb added a commit to MinchinWeb/pelican that referenced this pull request Apr 23, 2020

Change default settings to be built with pathlib.Path. Closes getpe…

7b18a89

…lican#2431. c.f. getpelican#2383.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert FEED setting from `%s` to `{slug}`-style strings #2383

Convert FEED setting from `%s` to `{slug}`-style strings #2383

MinchinWeb commented Jul 12, 2018 •

edited

Loading

justinmayer commented Jul 12, 2018

MinchinWeb commented Jul 13, 2018 •

edited

Loading

justinmayer commented Aug 3, 2018

mosra commented Sep 12, 2018

MinchinWeb commented Sep 18, 2018

MinchinWeb commented Sep 18, 2018 •

edited

Loading

mosra Sep 19, 2018

MinchinWeb Sep 20, 2018

avaris commented Oct 2, 2018

MinchinWeb commented Oct 4, 2018

avaris commented Oct 4, 2018

MinchinWeb commented Oct 13, 2018

justinmayer commented Oct 23, 2018

avaris commented Oct 30, 2018

justinmayer commented Nov 1, 2018

MinchinWeb commented Nov 2, 2018

avaris commented Nov 2, 2018

MinchinWeb commented Nov 3, 2018

Convert FEED setting from %s to {slug}-style strings #2383

Convert FEED setting from %s to {slug}-style strings #2383

Conversation

MinchinWeb commented Jul 12, 2018 • edited Loading

justinmayer commented Jul 12, 2018

MinchinWeb commented Jul 13, 2018 • edited Loading

justinmayer commented Aug 3, 2018

mosra commented Sep 12, 2018

MinchinWeb commented Sep 18, 2018

MinchinWeb commented Sep 18, 2018 • edited Loading

mosra Sep 19, 2018

Choose a reason for hiding this comment

MinchinWeb Sep 20, 2018

Choose a reason for hiding this comment

avaris commented Oct 2, 2018

MinchinWeb commented Oct 4, 2018

avaris commented Oct 4, 2018

MinchinWeb commented Oct 13, 2018

justinmayer commented Oct 23, 2018

avaris commented Oct 30, 2018

justinmayer commented Nov 1, 2018

MinchinWeb commented Nov 2, 2018

avaris commented Nov 2, 2018

MinchinWeb commented Nov 3, 2018

Convert FEED setting from `%s` to `{slug}`-style strings #2383

Convert FEED setting from `%s` to `{slug}`-style strings #2383

MinchinWeb commented Jul 12, 2018 •

edited

Loading

MinchinWeb commented Jul 13, 2018 •

edited

Loading

MinchinWeb commented Sep 18, 2018 •

edited

Loading