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

fix: Completely skip pages excluded by match_path #49

Merged
merged 4 commits into from
Mar 26, 2021
Merged

fix: Completely skip pages excluded by match_path #49

merged 4 commits into from
Mar 26, 2021

Conversation

prcr
Copy link
Contributor

@prcr prcr commented Mar 20, 2021

I was experimenting @rmorshea's great contribution on #43 to include only a small subset of relevant pages in the feeds for my site, hoping that it would be an elegant way to overcome the limitation in #23. However, I noticed the following:

  • The plugin was still outputting warnings because it failed to obtain the correct Git dates from pages that I was excluding with match_path
  • None of the pages that I was including with match_path would show up on the output

This was because the match_path filter was only being applied during the on_post_build event handler, and after the list of pages to include in the output had already been truncated by the configuration length.

To fix this issue I propose that we completely skip processing the pages that aren't included in match_path, with the additional advantages:

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 20, 2021
@codecov-io
Copy link

codecov-io commented Mar 20, 2021

Codecov Report

Merging #49 (6880ec8) into master (19f89e6) will decrease coverage by 0.20%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
- Coverage   37.66%   37.45%   -0.21%     
==========================================
  Files           5        5              
  Lines         300      299       -1     
  Branches       57       57              
==========================================
- Hits          113      112       -1     
  Misses        182      182              
  Partials        5        5              
Impacted Files Coverage Δ
mkdocs_rss_plugin/customtypes.py 100.00% <ø> (ø)
mkdocs_rss_plugin/plugin.py 33.33% <25.00%> (-0.39%) ⬇️
mkdocs_rss_plugin/util.py 25.15% <100.00%> (+<0.01%) ⬆️

@Guts
Copy link
Owner

Guts commented Mar 20, 2021

Thanks @pauloribeiro-codacy for taking time to contribute. I'll try to make a review in the next days.

@prcr
Copy link
Contributor Author

prcr commented Mar 25, 2021

(I inadvertently closed the pull request when I mentioned it in codacy/docs#567. 😅 )

In the meantime, I'm testing this fix on the site that I'm maintaining:

@prcr prcr reopened this Mar 25, 2021
@Guts Guts self-assigned this Mar 25, 2021
@Guts
Copy link
Owner

Guts commented Mar 25, 2021

It's working for me.

@pauloribeiro-codacy with this behavior, is it possible to match multiple folders? for example with your website, include release-notes and articles.

@prcr
Copy link
Contributor Author

prcr commented Mar 26, 2021

@Guts yes, since match_path gives you all the power of regular expressions you can have more complex patterns to include multiple directories. For example, to include all pages under both release-notes and articles:

    - rss:
          match_path: "(release-notes|articles)/.*"

@Guts
Copy link
Owner

Guts commented Mar 26, 2021

Ok thanks. I will add it for reference in the documentation.

@Guts Guts merged commit 9387117 into Guts:master Mar 26, 2021
prcr pushed a commit to codacy/docs that referenced this pull request Mar 27, 2021
The updated version includes the fix from
Guts/mkdocs-rss-plugin#49
prcr added a commit to codacy/docs that referenced this pull request Mar 27, 2021
@prcr prcr deleted the fix/filter-match-path branch March 27, 2021 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants