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

Extend PageMatcher with Environment keyword #9715

Merged
merged 3 commits into from
Apr 5, 2022

Conversation

CathrinePaulsen
Copy link
Contributor

Fixes #9612

This fix allows environment to be used as a keyword for cascade._target.
It can be used to apply cascade options to the target in only certain build environments, for example to specify in which environment(s) certain pages should not be rendered.

The documentation has been updated with the new keyword.

@@ -167,6 +167,10 @@ func (p *pageMeta) Description() string {
return p.description
}

func (p *pageMeta) Environment() string {
Copy link
Member

Choose a reason for hiding this comment

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

0K, this PR looks mostly good, but we don't want to add the Environment() everywhere -- the Page interface is big as it is.

So I suggest that you in the page matcher do something ala:

if m.Environment != "" {
		g, err := glob.GetGlob(m.Environment)
		if err == nil && !g.Match(p.Site().Hugo().Environment()) {
			return false
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! That makes sense, I'll change it.

The reason for the current implementation was to make extending the PageMatcher test as easy as possible, since otherwise it will panic when calling p.Site() since it's not implemented for testPage.

How do you suggest I change the PageMatcher test? Should I implement testPage.Site() or is there a better way?

Copy link
Member

@bep bep Mar 25, 2022

Choose a reason for hiding this comment

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

Yea, I see your point. Ultimately we may want to split it (e.g. having both a MatchesSite and a MatchesPage method or something), but even then you will have the same test challenge.

There is a testSite implementation in there somewhere, so I suggest you just expand on that. It makes for a little more work, but certainly a better end game.

Copy link
Contributor Author

@CathrinePaulsen CathrinePaulsen Apr 3, 2022

Choose a reason for hiding this comment

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

I think the most recent commit should address the requested change. Let me know what you think!

Implement the Environment keyword for PageMatcher in a more straightforward way.
The extended PageMatcher test is adjusted accordingly and the required missing test functionality is added.
@bep
Copy link
Member

bep commented Apr 5, 2022

Yea, now it looks good, thanks!

@bep bep merged commit da00e77 into gohugoio:master Apr 5, 2022
@github-actions
Copy link

github-actions bot commented Apr 6, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add environment (e.g. production) as a new filter to _cascade.target
2 participants