Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

Fix regenerating site for change in excluded files while tasting #265

Merged

Conversation

kodfodrasz
Copy link
Contributor

Taste mode does not trigger complete regeneration of the change of a file excluded in the configuration.

Fixes #264

@kodfodrasz
Copy link
Contributor Author

It was a bit too early to submit this PR. The first attempt for the fix was not correct :(

 - Use SiteContextGenerator.IsExcludedPath() for exclude detection instead of CanBeIncluded()
 - for this the method visibility had to be changed to public.
{
return excludes.Contains(relativePath) || excludes.Any(e => relativePath.StartsWith(e));
}

private bool IsIncludedPath(string relativePath)
public bool IsIncludedPath(string relativePath)
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I only changed visibility to be "semantically symmetric" with IsExcludedPath. Should I add unit test for this as well, ot revert this change?

Copy link
Member

Choose a reason for hiding this comment

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

You have a point about the "semantically symmatric", leave the change and just add some unit tests please.

@laedit
Copy link
Member

laedit commented Aug 18, 2015

Thanks for your PR!
I just have some concerns, see the comments on the commit.

@thoemmi
Copy link
Contributor

thoemmi commented Nov 19, 2015

I was just about to create a similar pull request when I stumbled over this one 😄
@kodfodrasz Are you going to add some unit tests? Otherwise I'll create a new complete PR.

@kodfodrasz
Copy link
Contributor Author

Hi!
Hang on, I'll try to get to finish this one on this weekend. It has been delayed slightly since I submitted it :) Now that there is someone interested in it i'll prioritize this.

offtopic note: the github "reply to this email to comment" feature did not quite work as intended. :(

@kodfodrasz
Copy link
Contributor Author

I hope these tests are enough. I don't think these methods need anything more complicated.

laedit pushed a commit that referenced this pull request Nov 23, 2015
…file-changes

Fix regenerating site for change in excluded files while tasting
@laedit laedit merged commit 072090d into Code52:master Nov 23, 2015
@laedit
Copy link
Member

laedit commented Nov 23, 2015

Thanks! 😃

@thoemmi
Copy link
Contributor

thoemmi commented Nov 23, 2015

👍 Thanks

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.

3 participants