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

Fixes #149 - PathResolver should implement an interface to enable mocking #150

Merged
merged 6 commits into from
Jul 13, 2016

Conversation

akerekes
Copy link
Contributor

No description provided.

@elharo
Copy link
Contributor

elharo commented Jun 25, 2016

I'm still not convinced this is the right approach. Let's talk on Monday.


import java.nio.file.Path;

public interface IPathResolver {
Copy link
Member

Choose a reason for hiding this comment

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

I would rename to plain PathResolver

@meltsufin
Copy link
Member

@akerekes @elharo Any interest in this still?

@akerekes
Copy link
Contributor Author

@meltsufin yes, we're still interested in this, but not sure whether this approach is the best. If we'll have an interface, the singleton constraint will be much more difficult to maintain, so we could just change it to a regular class (and not make it a singleton).

@meltsufin
Copy link
Member

I don't think it needs to be a singleton at the API level. This class doesn't have any state. So, it can either be a static utility method, or an interface and a default implementation. I prefer the latter, but making it a static method would not make it worse than it is now.

@akerekes
Copy link
Contributor Author

@elharo @meltsufin PTAL

@elharo
Copy link
Contributor

elharo commented Jul 13, 2016

I'm not sure why we need the interface here. Just rename DefaultPathResolver to PathResolver, and we're good. It's no longer an unmockable singleton, which was the concern.

@elharo
Copy link
Contributor

elharo commented Jul 13, 2016

LGTM

@meltsufin
Copy link
Member

👍

@akerekes akerekes merged commit 1f1e9c1 into master Jul 13, 2016
@akerekes akerekes deleted the i149 branch July 13, 2016 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants