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

Moving forward #212

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

Moving forward #212

gfosco opened this issue May 13, 2015 · 20 comments

Comments

@gfosco
Copy link
Contributor

@gfosco 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
Copy link
Contributor

@janoszen 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
Copy link
Contributor

@0x20h 0x20h commented May 13, 2015

+1

@OndraM
Copy link
Collaborator

@OndraM OndraM commented May 14, 2015

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

@janoszen
Copy link
Contributor

@janoszen janoszen commented May 14, 2015

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

@gabrielpreston
Copy link
Contributor

@gabrielpreston gabrielpreston commented May 14, 2015

I'm on board with this as well

@0x20h
Copy link
Contributor

@0x20h 0x20h commented May 15, 2015

@janoszen Why not prepare a PR?

@janoszen
Copy link
Contributor

@janoszen janoszen commented May 15, 2015

Any requirements on the namespace part?

@0x20h
Copy link
Contributor

@0x20h 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
Copy link
Contributor

@janoszen janoszen commented May 15, 2015

I can do that fairly quickly if that is ok.

@OndraM
Copy link
Collaborator

@OndraM OndraM commented May 15, 2015

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

@0x20h
Copy link
Contributor

@0x20h 0x20h commented May 15, 2015

@OndraM yep, absolutely. Thanks for the feedback!

@gfosco
Copy link
Contributor Author

@gfosco gfosco commented May 15, 2015

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

@janoszen
Copy link
Contributor

@janoszen 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
Copy link
Contributor

@0x20h 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
Copy link
Contributor

@janoszen janoszen commented May 19, 2015

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

@janoszen
Copy link
Contributor

@janoszen 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
Copy link
Contributor

@0x20h 0x20h commented May 19, 2015

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

@gfosco
Copy link
Contributor Author

@gfosco gfosco commented May 21, 2015

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

@drjayvee
Copy link
Contributor

@drjayvee drjayvee commented May 27, 2015

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
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
Pull request for #212
@gfosco
Copy link
Contributor Author

@gfosco gfosco commented Jun 19, 2015

Ok, we did it!.. 👍

brendeschuijmert pushed a commit to brendeschuijmert/php-webdriver that referenced this issue Jul 26, 2019
Implemented namespacing as discussed (needs testing)
brendeschuijmert pushed a commit to brendeschuijmert/php-webdriver that referenced this issue Jul 26, 2019
brendeschuijmert pushed a commit to brendeschuijmert/php-webdriver that referenced this issue Jul 26, 2019
Implemented namespacing as discussed (needs testing)
brendeschuijmert pushed a commit to brendeschuijmert/php-webdriver that referenced this issue Jul 26, 2019
brendeschuijmert added a commit to brendeschuijmert/php-webdriver that referenced this issue Apr 13, 2020
Implemented namespacing as discussed (needs testing)
brendeschuijmert added a commit to brendeschuijmert/php-webdriver that referenced this issue Apr 13, 2020
brendeschuijmert added a commit to brendeschuijmert/php-webdriver that referenced this issue Apr 13, 2020
Implemented namespacing as discussed (needs testing)
brendeschuijmert added a commit to brendeschuijmert/php-webdriver that referenced this issue Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.