Fix 134

[rvodden]

Resolution to issue #134

[paulgibbs]

I think we need to plan for the situation that we have traits to do other
things. If awareness sounds odd then how about Entity - EntityTraits might
be even better?

I'm not convinced. I don't want to introduce "Entity" because that's just a new name and we haven't found a use for that anywhere else. If a Context had a new trait, it'd go into the context/trait(s)/ folder, and if some non-Context object needed a trait, we'd just sub-namespace under that appropriately.

I think the options are between:

1. removing the proposed Awareness folder and namespace entirely and putting everything in to src/Context/ (like the existing PageObjectContextTrait.php, which I guess we'd rename to PageObjectAwareContextTrait.php for this new naming convention).

2. renaming Awareness to Trait or something else yet to be suggested.

[rvodden]

Entity is what the matching classes are called on the driver side. I'll rename the folder. Don't really mind what it's called :o)

…

On Fri, 20 Oct 2017, 16:23 Paul Gibbs, ***@***.***> wrote: I think we need to plan for the situation that we have traits to do other things. If awareness sounds odd then how about Entity - EntityTraits might be even better? I'm not convinced. I don't want to introduce "Entity" because that's just a new name and we haven't found a use for that anywhere else. If a Context had a new trait, it'd go into the context/trait(s)/ folder, and if some non-Context object needed a trait, we'd just sub-namespace under that appropriately. I think the options are between: 1. removing the proposed Awareness folder and namespace entirely and putting everything in to src/Context/ (like the existing PageObjectContextTrait.php, which I guess we'd rename to PageObjectAwareContextTrait.php for this new naming convention). 2. renaming Awareness to Trait or something else yet to be suggested. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#144 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADO2yfwahRLtTYFHyBFZDqAOfEZdwCToks5suLr0gaJpZM4P8nvW> .

[paulgibbs]

You might be thinking of Element. On 20 October 2017 at 16:25, Richard Vodden <notifications@github.com> wrote:

…

Entity is what the matching classes are called on the driver side. I'll rename the folder. Don't really mind what it's called :o) On Fri, 20 Oct 2017, 16:23 Paul Gibbs, ***@***.***> wrote: > I think we need to plan for the situation that we have traits to do other > things. If awareness sounds odd then how about Entity - EntityTraits might > be even better? > > I'm not convinced. I don't want to introduce "Entity" because that's just > a new name and we haven't found a use for that anywhere else. If a Context > had a new trait, it'd go into the context/trait(s)/ folder, and if some > non-Context object needed a trait, we'd just sub-namespace under that > appropriately. > > I think the options are between: > > 1. > > removing the proposed Awareness folder and namespace entirely and > putting everything in to src/Context/ (like the existing > PageObjectContextTrait.php, which I guess we'd rename to > PageObjectAwareContextTrait.php for this new naming convention). > 2. > > renaming Awareness to Trait or something else yet to be suggested. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/paulgibbs/behat-wordpress-extension/ pull/144#issuecomment-338238429>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ ADO2yfwahRLtTYFHyBFZDqAOfEZdwCToks5suLr0gaJpZM4P8nvW> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#144 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABN4Crd_d2gj6wwx91aDIvUO2-VjPlxCks5suLtagaJpZM4P8nvW> .

-- Regards, Paul Gibbs

[rvodden]

ahah - EXACTLY - serves me right for making suggestions without looking at the code base. What do you think of ElementTraits?

[paulgibbs]

they’re not really traits for an element, they’re for a context. But. Given we are only talking about the name of part of a namespace and a folder name, I like it better than Awareness by lots.

[rvodden]

Right @paulgibbs all yours! I have a couple of comments:

1. You'll notice I've introduced a few scrutinizer issues along the lines of "Accessing cache on the interface PaulGibbs\WordpressBehat...\Driver\DriverInterface suggest that you code against a concrete implementation. How about adding an instanceof check?". There are a few ways to fix these - but I'd like to decouple it from this pull request. I'll raise a separate feature request where we can discuss the options around that.
2. You'll also notice the traits directory is now called traits. I tried trait as you suggested, but unfortunately it's a reserved word, so can't form part of a namespace. I figured traits was next best.

Now all of the driver agnostic code is in the traights. All the driver specific code is in the drivers. Let me know what you think - I've been pretty thorough with scrutinizer and codacy, but I dare say I've missed something. :-)

[paulgibbs]

In reply to yours:

1. There were lots of these pre-existing. I ignored them as I couldn't see how to make it better. I believe this PR will reduce the quantity of those, so it's heading in the right direction, and if you've got further ideas, like you say another ticket is the perfect place.

2. Fair enough.

Also:

3. EditPostContext, "edit_post_page" - "TODO: this needs to not be public!" (was private) -- this doesn't need fixing for this PR, but if you could open an issue with your thought process behind this change and your comment, we can figure it out in the future.

4. In CommentAwareContextTrait (and more places) deleteComment(), you've renamed the first arg from $id to $commentId. I don't mind the renaming, but the variable name convention the project is using means this should be $comment_id. We can go through and fix this in a subsequent change, don't worry about it for now (in a perfect world, i'd like PHPCS to check these, but I don't know if that's possible).

5. There's a couple of very minor whitespacing issues but i'm being ultra picky now so i'm not going to call them out. :) Such things can be iterated next time the relevant file is touched.

Great work, again!

[paulgibbs]

@rvodden I've told Github to give you push access to this repository. Once you have it, please merge this PR in. I don't want to become a roadblock or speed bump to any change you may wish to contribute in the future. :)

If you can jump on our Slack channel, we can have a conversation sometime about how we commit to master, but in a nutshell: 1) don't worry about breaking things, 2) please do not tag new releases, 3) never commit to master, 4) be patient and try hard to let someone else code review/merge your branches, and finally 5) know when to break these rules (great power comes with great responsibility).

Thanks again :)

[rvodden]

Thanks Paul - a very sensible set of rules. I'm delighted to be on board.