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

Make file name matcher check on base name of entry #7067

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ainopara
Copy link

@ainopara ainopara commented Jun 8, 2018

I am writing a textbundle plugin for jekyll. I found that If a folder in _posts named 2018-06-09-Test.textbundle, which can pass the matcher check, all files in the folder will pass the check no matter what name they have.

That means all the png images in textbundle will tried to be read by Jekyll and we will see an error message for them. textbundle format also contains a json file which is not used by Jekyll but recognized as a Jekyll::Document.

I know having a folder in _posts folder and it’s name happened to pass the file name checker is not common, but file name checker is supposed to check a file’s name rather than it’s location. So I think it will be nice to fix it.

If  a folder in `_posts` named `2018-06-09-Test.textbundle`, which can pass the matcher check, and all files in the folder will pass the check no matter what name they have.
@ashmaroli
Copy link
Member

@ainopara Will you be able to write some tests for this..?

@kenman345
Copy link
Contributor

@ainopara This looks pretty solid to me. Can you show me the issue you're having in a repository so I can mock that up for tests to help you along?

@ainopara
Copy link
Author

@kenman345 Thank you for helping me to write tests for this change. It's beyond my ability to write tests for this method on my own.
Is https://github.com/ainopara/BugReportDemo/tree/jekyll/issue-7067 sufficient for this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants