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

Plugin system #1407

Closed
wants to merge 6 commits into from
Closed

Plugin system #1407

wants to merge 6 commits into from

Conversation

simov
Copy link
Member

@simov simov commented Feb 5, 2015

Closes #1400
Closes #1406

I just merged #1400 and #1406 here + refactored all of the 4 plugins to use similar structure.

@nylen
Copy link
Member

nylen commented Feb 5, 2015

I like the concept, this is definitely the direction we want to head IMO. I'm a little worried about unexpectedly breaking things though, like #1409.

I have been swamped lately (just accepted a job with Automattic!) so I'll need at least a few days to do a proper review. @mikeal @FredKSchott @seanstrom etc. any help appreciated.

@simov
Copy link
Member Author

simov commented Feb 5, 2015

I'm really not changing that much at this stage, just moving code around, the instance bug with auth is very silly, but apparently not caught by any of the tests.

@nylen
Copy link
Member

nylen commented Feb 5, 2015

I agree, I feel like I could have caught that in reviewing though, so I'd like to be more thorough this time.

@simov
Copy link
Member Author

simov commented Feb 5, 2015

OK, my point in #1409 was that it's about code modified in this PR + I already merged two other PR's here, so I don't want to push even more things here :)

EDIT: Congrants on the new job @nylen !

@mikeal
Copy link
Member

mikeal commented Feb 5, 2015

I don't have time to do a full review but I will say that I like this approach. I'll be impressed if you can actually work the forwarding logic in to a plugin, that logic always seemed to infect parts of the code I never thought I'd have to worry about :)

@simov
Copy link
Member Author

simov commented Feb 5, 2015

@mikeal I'm not familiar with the entire code base in details yet tbh, but still the refactoring should start from somewhere.

@lalitkapoor
Copy link
Member

@nylen off topic, but congrats on the new job!! :)

@seanstrom
Copy link
Contributor

I'll be doing a thorough review today :)

@simov
Copy link
Member Author

simov commented Feb 7, 2015

Thanks @seanstrom take a look at this PR #1406 too, as most of my latest ideas are there. Also not really that much changed at this point, I'm really just moving code into separate scopes as I see fit. For example the last one - the redirect one is just something that I thought it deserves it's own scope, and in fact except two of the fields in this newly created class are used externally, so it looks like a separate entity after all.

@simov simov mentioned this pull request Feb 8, 2015
@nylen nylen closed this in #1413 Feb 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants