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

Experiment (for review) #3

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wasnotrice
Copy link
Member

As a first exploration of brown shoes, I tried moving Swt into the Shoes namespace, so SwtShoes::Button becomes Shoes::Swt::Button and so on. Motivation: make the various flavors of Shoes feel like one integrated project (from a user perspective).

require 'shoes/swt'

vs

require 'swt_shoes'

But mainly I'm just getting my feet wet with the code. @pjfitzgibbons what do you think?

@pjfitzgibbons
Copy link
Member

Did that once and it got very ugly.
Also discovered that the multi-GUI framework need more decoupling.
You'll notice in the current HEAD that the frameworks are completely
separate from the base, and that the frameworks provide implementations to
the hooks that are called in Shoes.

May I fwd this to ML for wider discussion?

And thank you sooo much for looking at this! You represent the first
code-review and think-through, and I appreciate it greatly.

Thanks,
Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

@wasnotrice
Copy link
Member Author

I really like the way you have decoupled the frameworks. I do not actually envision the code ending up all in the same repo, just in the same namespace. It could be similar to datamapper, as an example, where you can get the whole ball of wax, or just the pieces you need. I didn't re-couple anything in this refactor, just adjusted the namespacing.

I can see how it could get messy--in fact, I had thought that it wouldn't work, until I started reading your code. Having some this exercise, I do think it's possibly to use a single-rooted Shors namespace and maintain the decoupling you have established. Perhaps it's just a matter of style.

Re: ML, of course, forward away! It might be helpful to get some more insights

@pjfitzgibbons
Copy link
Member

Eric, is this change a dependency of your subsequent pull requests?
Wondering if I absolutely need to merge this immediately ?

@wasnotrice
Copy link
Member Author

Peter, no, you do not need to merge this one. The other work is done within the pre-existing naming scheme. This was intended as a conversation starter :)

@pjfitzgibbons
Copy link
Member

I'd be ok with moving on this one. I didn't originally understand your
change. I think your change is ok.
Tell me if you'd like help on this one.

Kindest Regards,
Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Thu, Apr 26, 2012 at 5:39 PM, Eric Watson <
reply@reply.github.com

wrote:

Peter, no, you do not need to merge this one. The other work is done
within the pre-existing naming scheme. This was intended as a conversation
starter :)


Reply to this email directly or view it on GitHub:
#3 (comment)

@wasnotrice
Copy link
Member Author

At this point, I would recommend that we work on the other pull requests first, and then come back to this one. The merges will be painful if we do this one first. It won't be hard to extend this naming change out if it's what we decide to do.

@pjfitzgibbons
Copy link
Member

Agreed
Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Fri, Apr 27, 2012 at 11:18 AM, Eric Watson <
reply@reply.github.com

wrote:

At this point, I would recommend that we work on the other pull requests
first, and then come back to this one. The merges will be painful if we do
this one first. It won't be hard to extend this naming change out if it's
what we decide to do.


Reply to this email directly or view it on GitHub:
#3 (comment)

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