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

Feature/API change request: expose failure_slash_error_lines data in ExceptionPresenter #2169

Closed
jzohrab opened this issue Feb 8, 2016 · 7 comments

Comments

@jzohrab
Copy link

jzohrab commented Feb 8, 2016

ServerSpec is currently monkeypatching the class and accessing this internal method (ref https://github.com/mizzy/serverspec/blob/master/lib/serverspec.rb#L46), a change to which has broken ServerSpec. Ref https://twitter.com/kantrn/status/664913752832126976, it's not good to monkeypatch a private API. I've submitted a patch to work around the current failure, but it would be more stable if ServerSpec could pull data from a published API.

Class being monkeypatched:
https://github.com/rspec/rspec-core/blob/master/lib/rspec/core/formatters/exception_presenter.rb

If it is better to work with an existing method in the public API, please advise which one is best. Thanks!

@JonRowe
Copy link
Member

JonRowe commented Feb 8, 2016

That entire class is private api because it's just used for our internal formatting, if serverspec needs to format data differently to us they should implement their own formatting, even if this is just vendoring our code as rebadging it as theres.

@JonRowe JonRowe closed this as completed Feb 8, 2016
@myronmarston
Copy link
Member

I think failure_slash_error_line would be a pretty odd public API but perhaps we can provide some other APIs. What API would help you?

@jzohrab
Copy link
Author

jzohrab commented Feb 8, 2016

Thanks @JonRowe. Too bad Ruby doesn't have private classes. Vendoring could create other trouble, I've not had good luck with it in the past - maintenance headaches.

@myronmarston - thanks for the offer. I'm not sure what the design choices were for this that necessitated the monkeypatching. I'll ask the project maintainer to pitch in here.

@JonRowe
Copy link
Member

JonRowe commented Feb 9, 2016

@jzohrab by vendoring in this instance I mean taking the code and making it there own, there would be no other trouble as the serverspec solution would then be independent rather than dependant on us, the only downside is they would have to maintain it themselves...

@jzohrab
Copy link
Author

jzohrab commented Feb 9, 2016

Thanks @JonRowe, that downside is the trouble I was referring to. If they vendor just that one class, I figured there may be other compatibility issues down the line as I think that you were saying that this class is internal only. Anyway, patched for now, and maybe the maintainer will reach out with more requests.

@JonRowe
Copy link
Member

JonRowe commented Feb 9, 2016

@jzohrab Yes they should implement there own formatters if they want to change how things are displayed, not just monkey patch ours, one way uses a public api and the maintenance overhead will be minimal unless they choose otherwise, this way we may continue to break things and we're not going to make APIs public to "support" monkey patches.

@jzohrab
Copy link
Author

jzohrab commented Feb 9, 2016

Thanks @JonRowe, appreciated!

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

3 participants