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

Allow multiple changelog files #61

Merged
merged 3 commits into from May 1, 2018

Conversation

MinchinWeb
Copy link
Contributor

@MinchinWeb MinchinWeb commented Sep 9, 2016

Fixes #59

Includes documentation updates;
Fixes bitprophet#59
@@ -629,6 +629,12 @@ def setup(app):
app.add_config_value(
name='releases_{0}'.format(key), default=default, rebuild='html'
)
# if a string is given for `document_name`, convert it to a list
# done to maintain backwards compatibility
if isinstance(app.config.releases_document_name, str):
Copy link
Owner

@bitprophet bitprophet Sep 9, 2016

Choose a reason for hiding this comment

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

Probably best to s/str/six.string_types/ here in case anyone's on Python 2 + using Unicode string objects.

@bitprophet
Copy link
Owner

bitprophet commented Sep 9, 2016

Curious why you're using "single-item tuple" style lists here? (i.e. ['foo', ] instead of ['foo']) - it's not standard style. Dunno if a linter would complain about it but I would if I was a linter :D

@bitprophet
Copy link
Owner

bitprophet commented Sep 9, 2016

Also, if you could add an integration test for this (there's existing ones for "use a different filename" for example) that'd be rad!

Besides that, the above nitpick, and the line comment I left, looks good - thanks!

@bitprophet bitprophet added this to the 1.3 milestone Sep 9, 2016
@MinchinWeb MinchinWeb force-pushed the multiple-projects branch 6 times, most recently from 2f2defa to 0c8abdc Compare Sep 10, 2016
@MinchinWeb
Copy link
Contributor Author

MinchinWeb commented Sep 10, 2016

I'm not sure where I picked up the "simple-item tuple"-style lists. Probably something wasn't turning into a list like I was wanting it to, so I made it a little more explicit. In any case, the code here as been updated to pass the linter :D

I added an integration test. That turned out to be at least twice as hard as adding the functionality in the first place. The biggest stumbling block proved to be that I couldn't figure out how to pass a python list via the command-line, so I had to set up the integration tests to accept an alternate configuration file and not to pass the -D option. But everything is working now!

@bitprophet
Copy link
Owner

bitprophet commented Sep 10, 2016

Huh, weird - not sure offhand why that was necessary, typically just "another directory w/ docs and conf.py" is enough! I might poke when I merge, but otherwise, glad you got it working :)

# (which by default is changelog.rst).
if app.env.docname != app.config.releases_document_name:
# (which by default is ['changelog.rst', ]).
if app.env.docname not in app.config.releases_document_name:
Copy link

@jta jta Nov 28, 2016

Choose a reason for hiding this comment

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

I think re.match(app.config.releases_document_name, app.env.docname) would be far more flexible here, since it would allow matching on */changelog.rst

Copy link
Contributor Author

@MinchinWeb MinchinWeb Nov 29, 2016

Choose a reason for hiding this comment

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

If you feed a list in as app.config.release_document_name, re breaks with the error TypeError: unhashable type: 'list'.

Alternately, if you feed */changelog.rst in as app.config.release_document_name, re breaks with the error sre_comstraints.error: nothing to repeat at position 0.

I feel like I'm missing something here in what you're proposing...

Copy link

@jta jta Nov 29, 2016

Choose a reason for hiding this comment

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

Sorry, it was off the cuff remark, I didn't check. The correct syntax based on that error message would be .*/changelog.rst rather than the glob syntax I used.

My point was that defining a list of files is cumbersome if you have a large project with an unknown set of changelogs. Regexes tend to be much more practical for such cases.

Copy link
Contributor Author

@MinchinWeb MinchinWeb Dec 30, 2016

Choose a reason for hiding this comment

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

I keep thinking on this. I like the idea of being able to simply specify a range of documents, but regex can be a pain to debug at the best of times.

Since this is passed from the Sphinx configuration file, which can execute arbitrary Python, would it make sense to have the user generate their own list somehow? Maybe add an example to the documentation.

Another option would be to pass it through a glob-er (is that even a word?) rather than a regex. Maybe this is done internally, maybe it is pushed to the config file. For example:

from pathlib import Path
p = Path('.')
all_changlogs = list(p.glob('**/changelog.rst'))
releases_document_name = [str(x) for x in all_changlogs]

Copy link
Owner

@bitprophet bitprophet Jan 2, 2017

Choose a reason for hiding this comment

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

Offhand I do prefer to leverage the fact that conf.py is a .py, so +1 to "just ask user to supply a list". Or the user could use glob.glob from stdlib, or third party modules like pathlib as noted.

tl;dr when a use case is uncommon-to-rare, enabling it via "user is enabled to write their own tiny bit of Python" is preferable over "we write our own 'blessed' tiny-bit-of-Python and expose as a func/feature".

So as long as we allow a list value in the setting, the user is free to generate it however they want, and we're neither assisting nor limiting them.

@bitprophet
Copy link
Owner

bitprophet commented May 1, 2018

Merging this now and oh no, the integration test is actually a false positive 😆 it only runs the 1st ('A') changelog; the 2nd is not actually built or run. Wish I'd noticed this before spending 15 minutes tearing my hair out over why a sanity 'break' of the B changelog was not resulting in a test failure...

@bitprophet
Copy link
Owner

bitprophet commented May 1, 2018

(FTR I am working on that and any other necessary fixes on my end.)

@bitprophet bitprophet merged commit ed6a130 into bitprophet:master May 1, 2018
1 check passed
@bitprophet
Copy link
Owner

bitprophet commented May 1, 2018

Solved that, made tweaks, applied to my real world use case, seems to work! Merged, will release as 1.5.0 soon.

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