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

Warn on command-line with permalink conflict #8342

Merged
merged 2 commits into from
Aug 16, 2020

Conversation

SeekingMeaning
Copy link
Contributor

This is a 🙋 feature or enhancement.

Summary

This prints a warning for permalink conflict when running jekyll serve

Example output:

Configuration file: ~/permalink-conflict-test/_config.yml
            Source: ~/permalink-conflict-test
       Destination: ~/permalink-conflict-test/_site
 Incremental build: disabled. Enable with --incremental
      Generating... 
       Jekyll Feed: Generating feed for posts
          Conflict: The URL '~/permalink-conflict-test/_site/z.html' is the destination for the following pages: b.markdown, c.markdown
                    done in 0.324 seconds.
 Auto-regeneration: enabled for '~/permalink-conflict-test'
    Server address: http://127.0.0.1:4000/
  Server running... press ctrl-c to stop.

Context

Closes #6207

@ashmaroli
Copy link
Member

@SeekingMeaning Thank you for submitting this pull request.
I'd like some changes to be made, however:

  • The Jekyll::Commands::Doctor.conflicting_urls is already a public class method. Why not call it directly from within the class Site instead of moving it into a new utility module?
    # lib/jekyll/site.rb
    
    def process
      ...
      Jekyll::Commands::Doctor.conflicting_urls(self)
    end
    The benefits being:
    • use of existing implementation — no need for additional tests.
    • doesn't break semver principle by the Doctor command class not being able to call the method directly from outside the class.
  • Secondly, how about issuing the warning as part of the Jekyll::Site#write method call instead of after writing all files?

@ashmaroli
Copy link
Member

Thanks for the changes.

..as part of the Jekyll::Site#write method call instead of after writing all files?

This has still not been resolved, though. The focus being instead of after writing all files.
I recommend that the warning be issued before the call to block attached to each_site_file.

@ashmaroli ashmaroli added this to the 4.2 milestone Aug 16, 2020
Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

LGTM!

@ashmaroli
Copy link
Member

@SeekingMeaning If you're interested, you can try and add a Cucumber based test to take this up to 💯
However, its totally your call and as far as I'm concerned, the current diff is sufficient for a merge.
Thank you for your contribution.

@ashmaroli ashmaroli requested a review from DirtyF August 16, 2020 06:36
@SeekingMeaning
Copy link
Contributor Author

Hmm where would be a good place to add the test?

@ashmaroli
Copy link
Member

where would be a good place to add the test?

features/permalinks.feature

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@ashmaroli
Copy link
Member

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit a4f5b85 into jekyll:master Aug 16, 2020
jekyllbot added a commit that referenced this pull request Aug 16, 2020
@SeekingMeaning

This comment has been minimized.

@ashmaroli

This comment has been minimized.

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

Successfully merging this pull request may close these issues.

Warn/error on command-line with permalink conflict
4 participants