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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert Commands::Serve#server_address signature change. #5456

Merged
merged 2 commits into from Oct 6, 2016
Merged

Revert Commands::Serve#server_address signature change. #5456

merged 2 commits into from Oct 6, 2016

Conversation

parkr
Copy link
Member

@parkr parkr commented Oct 6, 2016 •
edited

This reverts a tiny part of #5431, which introduced a backwards-incompatible change that JekyllAdmin was relying on. It's never fun to break things and jekyll-admin must have a stunning reputation for not breaking. 馃槃

This is also necessary because launch_browser also used server_address and wasn't updated. So we have to handle that anyway.

/cc @jekyll/stability

@parkr parkr added the fix label Oct 6, 2016
@parkr parkr added this to the 3.3 milestone Oct 6, 2016
@jekyllbot jekyllbot assigned ghost and parkr Oct 6, 2016
@parkr
Copy link
Member Author

parkr commented Oct 6, 2016

/cc @benbalter as well.

envygeeks
envygeeks approved these changes Oct 6, 2016
View reviewed changes
Copy link
Contributor

@envygeeks envygeeks left a comment

Choose a reason for hiding this comment

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

Okay.

@benbalter
Copy link
Contributor

馃憤 this doesn't fix the breaking change for Jekyll Admin, but we were monkey patching the process method, so I'm fine with pushing the complexity into JekyllAdmin here, doing what's best for Jekyll, and making the necessary change upstream.

ashmaroli
ashmaroli approved these changes Oct 6, 2016
View reviewed changes
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.

Fixes #5451, so 馃憤

@parkr
Copy link
Member Author

parkr commented Oct 6, 2016

The breaking change was the creation of the start_webrick method, which I think we should keep. Monkey-patching is not really something we should feel we need to support, anyway, and Ben is fine with pushing it upstream. I'd love to get a jekyll-admin fix out before 3.3.

@ashmaroli
Copy link
Member

FWIW, v3.3.0 doesnt break jekyll-admin with this change incorporated..

@parkr
Copy link
Member Author

parkr commented Oct 6, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 21cd382 into master Oct 6, 2016
@jekyllbot jekyllbot deleted the revert-server-address-sig-change branch October 6, 2016 17:36
jekyllbot added a commit that referenced this pull request Oct 6, 2016
@jekyllbot
Update history to reflect merge of #5456 [ci skip]
dc554c2
@jekyll jekyll locked and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@envygeeks envygeeks envygeeks approved these changes

@ashmaroli ashmaroli ashmaroli approved these changes

Assignees

@parkr parkr

Labels
fix frozen-due-to-age
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@parkr @benbalter @ashmaroli @envygeeks @jekyllbot