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

CamelcasedClassNameResolver now can resolve FQCN page and element #81

Merged
merged 2 commits into from
Feb 24, 2017

Conversation

DonCallisto
Copy link
Contributor

Issue ref.: #76

Don't know if specs are good ones, could you check @jakzal?

Thanks :)

@DonCallisto
Copy link
Contributor Author

If a totally new and separated one resolver is required (it was my first idea, but reading your suggestion drove me to this implementation) let me know.

@DonCallisto
Copy link
Contributor Author

@jakzal now build is passing, let me know if there is something else I need to do or if this need changes.

Peace!

Copy link
Member

@jakzal jakzal left a comment

Choose a reason for hiding this comment

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

Would it be possible to add a scenario covering this new behaviour?

}
}

class Foo extends Page
Copy link
Member

Choose a reason for hiding this comment

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

These two classes (Foo and Bar) are not used. Please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakzal I'm using them in spec examples to validate if it's a valid FQCN

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean arguments passed to the class under test? These are only namespace prefixes. In your case these would be ignored, as a FQCN is passed to the resolve method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean this

private function isPageObjectNameFQCN($pageObjectName)
{
  return class_exists($pageObjectName);
}

If I remove those classes, spec examples I add will fail

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, can you use spec\\SensioLabs\\Behat\\PageObjectExtension\\PageObject\\Factory\\Fixtures\\NamespacedArticleList and spec\\SensioLabs\\Behat\\PageObjectExtension\\PageObject\\Factory\\Fixtures\\Element\\NamespacedSearchBox instead of the new ones? They're used in other examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Umh, yes, but in that case I could not tell the difference in the examples, I mean, they're going to work even if FQCN part is broken. Maybe I can create brand new ones that not respects Page/Element structures in Fixtures folder?

Copy link
Member

Choose a reason for hiding this comment

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

There's still a difference, as in the existing examples only part of the namespace is passed (and a whole namespace is returned with prefixes). In your new examples, you'd be passing a full namespace in, and expecting it back.


throw new \InvalidArgumentException($message);
if ($this->isPageObjectNameFQCN($pageObjectName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to move this condition to the beginning of the method? We wouldn't have to evaluate other parts of this method if the passed argument is already a class name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I would have to but I was not sure about that. Indeed is far better.

@jakzal
Copy link
Member

jakzal commented Feb 7, 2017

We will also need to document this new behaviour :)

@DonCallisto
Copy link
Contributor Author

We will also need to document this new behaviour :)

Yes, you're right! :)

@DonCallisto
Copy link
Contributor Author

I've made some edits. If everything is ok I'll edit the documentation also. Let me know :)

@jakzal
Copy link
Member

jakzal commented Feb 7, 2017

Looks great! A scenario (if easy to add), and docs would be appreciated :)

@DonCallisto
Copy link
Contributor Author

Don't know if I understand correctly all behat scenarios you already have (going to take a look later).
Docs will follow ;)

@DonCallisto
Copy link
Contributor Author

I've updated docs, please review them @jakzal

Thanks :)

- working with elements
- working with page objects

Introduction to FQCN resolver for page/element factory
@DonCallisto
Copy link
Contributor Author

I've also introduced a scenario adapted from existing one

@DonCallisto
Copy link
Contributor Author

@jakzal ping :)

@jakzal jakzal merged commit 8405541 into sensiolabs:master Feb 24, 2017
@jakzal
Copy link
Member

jakzal commented Feb 24, 2017

Thank you @DonCallisto. Excellent work! 🍺

@DonCallisto
Copy link
Contributor Author

DonCallisto commented Feb 24, 2017

It was a pleasure: you're welcome! :)

}

If you choose FQCN strategy, you can organize your page directories freely as you are not bounded to page namespace
(see `configuration <http://behat-page-object-extension.readthedocs.io/en/latest/guide/configuration.html>`_)
Copy link
Member

Choose a reason for hiding this comment

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

I converted this link to an internal reference to make docs more portable: #83

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

2 participants