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

Moving forward #212

Closed
gfosco opened this Issue May 13, 2015 · 20 comments

Comments

Projects
None yet
6 participants
@gfosco
Contributor

gfosco commented May 13, 2015

In order to support both Facebook internal use and acceptance in the broader community, and considering the slow current slow pace of development (last release tagged 7 months ago,) we should maintain an official community version which is allowed to grow on its own.

This way, the community can support current practices and interoperate better with other tools. Important bug-fixes can be made to both versions, and everyone could help review and improve the code. There are many forks, and some of that can be shared back for the benefit of all.

I've created a branch, community, which can become the default branch. How about we use that to comply with PSR-2 and add Namespaces, per #204 and #190, and make a 1.0 release?

Any thoughts on this approach?

@janoszen

This comment has been minimized.

Show comment
Hide comment
@janoszen

janoszen May 13, 2015

Contributor

I have a working version with namespaces here: https://github.com/opsbears/php-webdriver

Saves you some work, just search and replace as needed. :)

Contributor

janoszen commented May 13, 2015

I have a working version with namespaces here: https://github.com/opsbears/php-webdriver

Saves you some work, just search and replace as needed. :)

@0x20h

This comment has been minimized.

Show comment
Hide comment
@0x20h

0x20h May 13, 2015

Contributor

+1

Contributor

0x20h commented May 13, 2015

+1

@OndraM

This comment has been minimized.

Show comment
Hide comment
@OndraM

OndraM May 14, 2015

Collaborator

+1, though this would mean rewriting our tests (or at least doing lots of search & replace).

Collaborator

OndraM commented May 14, 2015

+1, though this would mean rewriting our tests (or at least doing lots of search & replace).

@janoszen

This comment has been minimized.

Show comment
Hide comment
@janoszen

janoszen May 14, 2015

Contributor

@OndraM Please do check my fork, it's all done.

Contributor

janoszen commented May 14, 2015

@OndraM Please do check my fork, it's all done.

@gabrielpreston

This comment has been minimized.

Show comment
Hide comment
@gabrielpreston

gabrielpreston May 14, 2015

Contributor

I'm on board with this as well

Contributor

gabrielpreston commented May 14, 2015

I'm on board with this as well

@0x20h

This comment has been minimized.

Show comment
Hide comment
@0x20h

0x20h May 15, 2015

Contributor

@janoszen Why not prepare a PR?

Contributor

0x20h commented May 15, 2015

@janoszen Why not prepare a PR?

@janoszen

This comment has been minimized.

Show comment
Hide comment
@janoszen

janoszen May 15, 2015

Contributor

Any requirements on the namespace part?

Contributor

janoszen commented May 15, 2015

Any requirements on the namespace part?

@0x20h

This comment has been minimized.

Show comment
Hide comment
@0x20h

0x20h May 15, 2015

Contributor

Just some thoughts:

  • Facebook\Webdriver as root namespace (other proposals?)
  • comply to PSR-4 (correct the autoload section in composer.json)
  • Rename WebdriverException to Facebook\Webdriver\Exception
  • Generally: follow one class per file
  • put exceptions under Facebook\Webdriver\Exception
Contributor

0x20h commented May 15, 2015

Just some thoughts:

  • Facebook\Webdriver as root namespace (other proposals?)
  • comply to PSR-4 (correct the autoload section in composer.json)
  • Rename WebdriverException to Facebook\Webdriver\Exception
  • Generally: follow one class per file
  • put exceptions under Facebook\Webdriver\Exception
@janoszen

This comment has been minimized.

Show comment
Hide comment
@janoszen

janoszen May 15, 2015

Contributor

I can do that fairly quickly if that is ok.

Contributor

janoszen commented May 15, 2015

I can do that fairly quickly if that is ok.

@OndraM

This comment has been minimized.

Show comment
Hide comment
@OndraM

OndraM May 15, 2015

Collaborator

@0x20h I would suggest keeping the name WebDriver. It is also official name of the W3C standard..

Collaborator

OndraM commented May 15, 2015

@0x20h I would suggest keeping the name WebDriver. It is also official name of the W3C standard..

@0x20h

This comment has been minimized.

Show comment
Hide comment
@0x20h

0x20h May 15, 2015

Contributor

@OndraM yep, absolutely. Thanks for the feedback!

Contributor

0x20h commented May 15, 2015

@OndraM yep, absolutely. Thanks for the feedback!

@gfosco

This comment has been minimized.

Show comment
Hide comment
@gfosco

gfosco May 15, 2015

Contributor

I'm 👍 on all of those suggestions @0x20h @OndraM

Contributor

gfosco commented May 15, 2015

I'm 👍 on all of those suggestions @0x20h @OndraM

@janoszen

This comment has been minimized.

Show comment
Hide comment
@janoszen

janoszen May 18, 2015

Contributor

OK guys, I have made the changes requested to my own repo (see above). It would need some real world testing, so if you have a project, feel free to jump on it. Feedback is also welcome.

Contributor

janoszen commented May 18, 2015

OK guys, I have made the changes requested to my own repo (see above). It would need some real world testing, so if you have a project, feel free to jump on it. Feedback is also welcome.

@0x20h

This comment has been minimized.

Show comment
Hide comment
@0x20h

0x20h May 18, 2015

Contributor

Hey @janoszen, thanks for your work!
Just had a quick look at it.
I think you should keep the directory structure and create sub namespaces.

Contributor

0x20h commented May 18, 2015

Hey @janoszen, thanks for your work!
Just had a quick look at it.
I think you should keep the directory structure and create sub namespaces.

@janoszen

This comment has been minimized.

Show comment
Hide comment
@janoszen

janoszen May 19, 2015

Contributor

Yeah, that wasn't clear. I'll get this done as soon as I have some time.

Contributor

janoszen commented May 19, 2015

Yeah, that wasn't clear. I'll get this done as soon as I have some time.

@janoszen

This comment has been minimized.

Show comment
Hide comment
@janoszen

janoszen May 19, 2015

Contributor

OK, it's done, but I've done an insane amount of automatic refactoring, so getting a second pair of eyes on the code would be good before the PR.

Contributor

janoszen commented May 19, 2015

OK, it's done, but I've done an insane amount of automatic refactoring, so getting a second pair of eyes on the code would be good before the PR.

@0x20h

This comment has been minimized.

Show comment
Hide comment
@0x20h

0x20h May 19, 2015

Contributor

FYI, our testsuite seems fine with your branch.
Let's move the discussion to the PR :)

Contributor

0x20h commented May 19, 2015

FYI, our testsuite seems fine with your branch.
Let's move the discussion to the PR :)

@gfosco

This comment has been minimized.

Show comment
Hide comment
@gfosco

gfosco May 21, 2015

Contributor

Can we get that submitted as a PR against the community branch here?

Contributor

gfosco commented May 21, 2015

Can we get that submitted as a PR against the community branch here?

@drjayvee

This comment has been minimized.

Show comment
Hide comment
@drjayvee

drjayvee May 27, 2015

I welcome this approach, as it allows for more centralized development.

I have one tiny PR to contribute: #217

I welcome this approach, as it allows for more centralized development.

I have one tiny PR to contribute: #217

0x20h added a commit to 0x20h/php-webdriver that referenced this issue May 29, 2015

Issue facebook/php-webdriver#212 - Moving forward
Implemented namespacing as discussed (needs testing)

0x20h added a commit to 0x20h/php-webdriver that referenced this issue May 29, 2015

gfosco added a commit that referenced this issue Jun 9, 2015

@gfosco

This comment has been minimized.

Show comment
Hide comment
@gfosco

gfosco Jun 19, 2015

Contributor

Ok, we did it!.. 👍

Contributor

gfosco commented Jun 19, 2015

Ok, we did it!.. 👍

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