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

Get version in docs without pkg_resources #262

Closed
wants to merge 2 commits into from

Conversation

davidfischer
Copy link
Contributor

  • Reads the version from icalendar/__init__.py
  • Appends the path to icalendar to the Python path like how the sphinx-quickstart sample conf.py recommends

This should fix the docs build on Read the Docs which hasn't built in a few years.

@davidfischer
Copy link
Contributor Author

Any way to get this a look? Read the Docs is getting reports of broken docs because the docs haven't been built in a few years there.

Copy link
Member

@thet thet left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!
But I have three remarks which should be addressed - one is adding a changelog entry to CHANGES.rst, the other two are shown below.

docs/conf.py Outdated
for line in f.readlines():
if line.startswith('__version__'):
version_info = {}
exec(line, None, version_info)
Copy link
Member

Choose a reason for hiding this comment

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

I dislike using exec here.
Please refactor to do it like here: https://github.com/collective/icalendar/blob/master/setup.py#L7

docs/conf.py Outdated
on_rtd = os.environ.get('READTHEDOCS', None) == 'True'

sys.path.append(os.path.join(base_dir, 'src'))
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed for a successful RTD build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it isn't strictly. I added this because I removed the . line from your requirements file. I believe editing the sys path is the preferred way of adding the module to the path. Depending on your setup, you may not need this.

@davidfischer
Copy link
Contributor Author

I made the change based on your feedback.

@fuzzy76
Copy link

fuzzy76 commented Feb 27, 2019

Is there anything particular holding this up? Working documentation would be nice...

@mgrollins
Copy link

I'm getting the same bad CSS on the built Doc Pages. https://icalendar.readthedocs.io/en/latest/ won't load CSS due to utilizing an old CDN that isn't supported by RTD any longer.

readthedocs/readthedocs.org#4082 (comment)

@roberson-io
Copy link

@davidfischer Sorry for the blast from the past, but it looks like this helpful fix you made a year ago may be stuck because the build requires an update the Changelog. Would you mind pushing an update and seeing if that helps move this along? I appreciate your help.

@davidfischer
Copy link
Contributor Author

@roberson-io, I created an alternative PR that may be preferable to this one #289. That one only specifies a Read the Docs config file rather than editing the conf.py. I'll make sure the tests pass over there but if that is merged this one can be closed. If instead you prefer this approach I can update this one.

@mauritsvanrees
Copy link
Member

https://icalendar.readthedocs.io is building correctly now, after PR #289 .

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

6 participants