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 Aug 10, 2018
@gwax gwax force-pushed the config_bundles branch from d3b0536 to 6a93712 Aug 10, 2018
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

@Kwpolska Kwpolska Aug 10, 2018

Choose a reason for hiding this comment

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

as well as*

Loading

Copy link
Member

@Kwpolska Kwpolska left a comment

LGTM (with comments)

Loading

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

@Kwpolska Kwpolska Aug 10, 2018

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.

Loading

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

@Kwpolska Kwpolska Aug 10, 2018

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)

Loading

Copy link
Contributor Author

@gwax gwax Aug 10, 2018

Choose a reason for hiding this comment

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

Allowed but not necessary.

Loading

@gwax
Copy link
Contributor Author

@gwax gwax commented Aug 10, 2018

Updated according to feedback.

Loading

@Kwpolska Kwpolska merged commit 961bb42 into master Aug 10, 2018
0 of 4 checks passed
Loading
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Aug 10, 2018

Thanks for contributing this!

Loading

@Kwpolska Kwpolska deleted the config_bundles branch Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants