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

Ignore git exceptions #20

Merged
merged 17 commits into from
Mar 27, 2020
Merged

Ignore git exceptions #20

merged 17 commits into from
Mar 27, 2020

Conversation

Guts
Copy link
Contributor

@Guts Guts commented Mar 26, 2020

Actual behavior

As specified in the readme:

The plugin needs access to the last commit that touched a file to be able to retrieve the date.

In certain cases (development phases, content edited by non developers...) where git is not installed or can't read files, it prevents the workflow.

By the way, the build should not fail because of a lack of third-party software.

Proposal

This PR adds an option to ignore git errors:

# mkdocs.yml
plugins:
  - git-revision-date-localized:
    locale: en
    ignore_missing_git: true  # false by default

@timvink
Copy link
Owner

timvink commented Mar 27, 2020

Hi @Guts,

Thanks for the PR! Lot of different improvements here.

The different changes make it a little bit harder to review. I would appreciate for next time a separate PR for things like 8f771b2.

By the way, the build should not fail because of a lack of third-party software.

I think it should when this plugin is enabled (PEP20):

Errors should never pass silently.
Unless explicitly silenced.

To be honest I have been struggling a bit with the fallback option when git is not available. Because the fallback is not longer the last revision date, but rather the last build date. This was confusing to users, because the dates might change depending on where you build your docs. Case in point: #10

After reviewing your PR I agree the cleanest way to deal with this is to be explicit about it with an option. I do think we should improve the naming of the parameter ignore_missing_git to better reflect the functionality. Do you have any suggestions? I was thinking fallback_to_build_date.

@Guts
Copy link
Contributor Author

Guts commented Mar 27, 2020

Hi @timvink

Thanks for the PR!

Thank you for your plugin, it's really well done and useful 👍

The different changes make it a little bit harder to review. I would appreciate for next time a separate PR for things like 8f771b2.

You're completely right. These changes are due to my automatic Python setup with pre-commit. I realized that and besides, my other pull requests are more 'atomic'.

If you prefer I can rollback on blacking and others EOF fixes, and only push the feature.

After reviewing your PR I agree the cleanest way to deal with this is to be explicit about it with an option. I do think we should improve the naming of the parameter ignore_missing_git to better reflect the functionality. Do you have any suggestions? I was thinking fallback_to_build_date.

Yeah I was not really inspired about the option name. Feel free to rename it with your idea.

@timvink
Copy link
Owner

timvink commented Mar 27, 2020

Thanks, happy the plugin is useful for others as well :)

If you prefer I can rollback on blacking and others EOF fixes, and only push the feature.

No need.

Feel free to rename it with your idea.

I don't have a lot of time today. If you'd like this merged quickly I'd appreciate some help with renaming to fallback_to_build_date :)

@Guts
Copy link
Contributor Author

Guts commented Mar 27, 2020

I don't have a lot of time today. If you'd like this merged quickly I'd appreciate some help with renaming to fallback_to_build_date :)

Here, confined = plenty of time to contribute to projects I use :)

@timvink timvink merged commit b6647ef into timvink:master Mar 27, 2020
@timvink
Copy link
Owner

timvink commented Mar 27, 2020

Looks good, thanks for all the work!

I'll wait for the other two PR's to complete because releasing a new version to PyPi

@timvink
Copy link
Owner

timvink commented Mar 28, 2020

Released 0.5.0

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