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

Does ExpressMappingStrategy belong here? #15

Closed
tpluscode opened this issue Oct 31, 2019 · 9 comments
Closed

Does ExpressMappingStrategy belong here? #15

tpluscode opened this issue Oct 31, 2019 · 9 comments

Comments

@tpluscode
Copy link
Contributor

I'm not sure how specific the ExpressMappingStrategy is to express actually?

I think right now that maybe it belongs to the http-problem-details-mapper package.

@AlexZeitler
Copy link
Contributor

AlexZeitler commented Oct 31, 2019

Currently it is not tied to express.

It is same as the HapiMappingStrategy here.

And even if someone would have the need to make it depend on express related stuff, it would still be possible by implementing a custom IMappingStrategy.

@tpluscode
Copy link
Contributor Author

My point was only that maybe the default implementation could be moved to the upstream packages. Just a thought

@AlexZeitler
Copy link
Contributor

Yes, I understood it that way and I wanted to express I agree with that 😄

@AlexZeitler
Copy link
Contributor

AlexZeitler commented Nov 13, 2019

Do you want to send a PR here or should I do it?

Would you mind reviewing the PRs then?

I would suggest it should be named DefaultMappingStrategy as abstract class MappingStrategy already exists in http-problem-details-mapper.

@tpluscode
Copy link
Contributor Author

👍 to the name but I would also move the implementation to the mapper package.

@AlexZeitler
Copy link
Contributor

Yes, from my understanding we already have committed to move it to the mapper package.

@tpluscode
Copy link
Contributor Author

Right, right. This is not a high priority for me right now. Go ahead and I will gladly review. Otherwise maybe can try over the weekend.

@AlexZeitler
Copy link
Contributor

No worries, I can do it. I just wanted to avoid misunderstandings.

@AlexZeitler
Copy link
Contributor

This is the PR for http-problem-details-mapper:
PDMLab/http-problem-details-mapper#14

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

No branches or pull requests

2 participants