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

Read bundles with configparser #3137

Merged
merged 4 commits into from Aug 10, 2018
Merged

Read bundles with configparser #3137

merged 4 commits into from Aug 10, 2018

Conversation

gwax
Copy link
Contributor

@gwax gwax commented Aug 10, 2018

Pull Request Checklist

  • I’ve read the guidelines for contributing.
  • I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable).
  • I tested my changes.

Description

This PR changes the file parser for bundles files from a one-off parser to a modified configparser parser. The parser is modified to assume an opening [bundles] section rendering it fully backwards compatible with the existing file format.

Doing so allows all existing files to parse as if nothing has changed and adds the full richness of configparser, importantly multiline values and comment (# or ;).

I have gone on to split the core theme bundles across multiple lines.

This does not introduce any new dependencies and maintains strict backwards compatibility with existing themes/bundles.

@gwax gwax requested a review from Kwpolska August 10, 2018 08:00
CHANGES.txt Outdated
@@ -14,6 +14,8 @@ Features
"{postDate} (${messages("updated")} {updateDate})". If no update
time is specified, the posting time will be displayed alone.
* All built-in themes now support the ``DATE_FANCINESS`` option.
* Theme bundles are now parsed using the configparser module and
can support newlines inside entries as well ad comments
Copy link
Member

Choose a reason for hiding this comment

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

as well as*

Copy link
Member

@Kwpolska Kwpolska left a comment

Choose a reason for hiding this comment

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

LGTM (with comments)

@@ -0,0 +1 @@
../base/bundles
Copy link
Member

Choose a reason for hiding this comment

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

That symlink should not be necessary, as bundles are inherited.

rst_base.css,
nikola_rst.css,
code.css,
theme.css,
Copy link
Member

Choose a reason for hiding this comment

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

Are the trailing commas (a) allowed or (b) necessary? (they are not in the manual examples)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allowed but not necessary.

@gwax
Copy link
Contributor Author

gwax commented Aug 10, 2018

Updated according to feedback.

@Kwpolska Kwpolska merged commit 961bb42 into master Aug 10, 2018
@Kwpolska
Copy link
Member

Thanks for contributing this!

@Kwpolska Kwpolska deleted the config_bundles branch August 10, 2018 14:23
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

2 participants