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

Use default name for parts #107

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@waiting-for-dev
Copy link
Contributor

waiting-for-dev commented Jan 7, 2019

It simplifies its instantiation and, therefore, unit testing them:

SomePart.new(value: SomeStruct)

WDYT @timriley ?

Further simplification could be made by using value: as standard argument, as opposed to keyword argument, but I don't consider it too critical.

Use default name for parts
It simplifies its instantiation and, therefore, unit testing them:

```
SomePart.new(value: SomeStruct)
```
@timriley

This comment has been minimized.

Copy link
Member

timriley commented Jan 8, 2019

Thanks for taking the time to think through this, @waiting-for-dev :)

Firstly, I think you might've tweaked the wrong method? I think you might be wanting #initialize (here) instead of #new.

Either way, I'm not sure about making the name optional, since it's used when building scope locals when rendering or building scopes. Leaving the name out e.g. in tests, when a name is always provided in the normal course of Part initialization, feels like it would make the tests less authentic and therefore less helpful. Does that make sense to you?

On a related note, I do still have an open issue about somehow supplying testing helpers (whether that's code we can ship in the gem or simply examples that can be copy/pasted, I'm not sure yet), but perhaps that's an opportunity to improve the ease of testing parts, rather than modifying their core code?

@waiting-for-dev

This comment has been minimized.

Copy link
Contributor Author

waiting-for-dev commented Jan 9, 2019

Thanks for your feedback @timriley

Firstly, I think you might've tweaked the wrong method? I think you might be wanting #initialize (here) instead of #new.

Oops, apologies... Terrible, I certainly prepared this PR to quick... Besides tweaking the wrong method, I made my tests working with last released version, where :rendering option was not present. Now I see that, for my idea to work, we should provide defaults for both :name and :rendering options.

Leaving the name out e.g. in tests, when a name is always provided in the normal course of Part initialization, feels like it would make the tests less authentic and therefore less helpful. Does that make sense to you?

I see your point. However, I've conflicting ideas with it. You are right with the fact that providing defaults would make the tests less authentic. But it is also true that the actual scenarios where parts are used are always in orchestration with other components. In a unit test, which is just another API consumer, it would make sense to be able to treat the part in isolation. If instead of being a functional object parts were just a module (with a pattern different that decorator, of course), :name and :rendering arguments would be required only in the methods where they are really used. You would not have troubles trying to test a simple "decorating" method.

On a related note, I do still have an open issue about somehow supplying testing helpers (whether that's code we can ship in the gem or simply examples that can be copy/pasted, I'm not sure yet), but perhaps that's an opportunity to improve the ease of testing parts, rather than modifying their core code?

To be honest, except for scenarios where complexity is high, I see the need of testing helpers as a code design smell. If there isn't a very good reason, I think objects should be always easy to instantiate. In fact, it is one of the main reason, together with state avoiding, why I think dry-view shines. dry-view controllers are a joy to instantiate and test. I think it would be great to have the same for parts 😃

If you agree with me, I can think of two possible solutions:

  • Be explicit with the defaults, like using :__anonymous__ for a default :name and a null object for :rendering. It would be the easiest solution to implement, but it has the drawback you commented.
  • Create a small wrapper around parts which would be responsible of orchestration needs, and leave a very bare decorator which would be instantiated only with the :value. With this decoupling we avoid the "real scenario" issue.

WDYT?

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