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

Only use public types in the signatures of exported formatter functions #463

Open
zachmandeville opened this issue Mar 9, 2022 · 3 comments

Comments

@zachmandeville
Copy link

zachmandeville commented Mar 9, 2022

🤔 What's the problem you've observed?

When extending a formatter (like the example emoji formatter), you can use the formatter.Storage methods to get back results
you need. However, a lot of these methods have signatures that rely on internal types. This can make it difficult to use these public methods.

For example, if i have formatter f, that is extending the godog.ProgressFmt, then i have access to this function:

f.Storage.MustGetPickleStepResultsByStatus

With this, I can easily get all failed steps, all passed steps, etc....

Except this function takes as its parameter models.StepResultStatus, and I do not have access to models. So if I want to get all passed steps, i have to know that models.Passed is roughly equivalent to 0 and pass 0 in directly.

This may be me misunderstanding how this function is intended to be used, though.

✨ Do you have a proposal for making it better?

My naive suggestion is to move models out of the internal directory, or at least adjust any of the base formatter functions to only use public types in their signatures.

@zachmandeville zachmandeville changed the title Make public the model types used in formatters Only use public types in the signatures of exported formatter functions Mar 9, 2022
@vearutop
Copy link
Member

vearutop commented Mar 9, 2022

For this particular case we have forwarded constants into public godog package: godog.StepPassed.

I think the idea of having most of implementation details under internal is to minimize API surface and so reduce a chance of backwards compatibility issues. Though at current state I'm not sure keeping internal really helps, as internal dependencies are already leaking into public API (for example f.Storage), maybe at this point we could export internal packages to give more powers (and responsibility) to developers. Before that it would be great to leverage breaking changes management with semantic import versioning (tag a v1.0.0), so that developers can safely rely on code.

@draganm
Copy link
Contributor

draganm commented May 14, 2022

I'm having similar issues as @zachmandeville - implementing any decent formatter requires access to internal packages. IMHO the whole way that formatters are build needs a revamp. In the ideal world, one should be able to build any of the 'internal' formatters based just on the Formatter interface. Having to reach into the Storage is leaking way too much of internals and begs the question about the purpose of the Formatter interface.

@vearutop
Copy link
Member

I agree, internal *formatters.Storage is a strong dependency that is vital for advanced formatting, maybe we can improve extensibility by abstracting it with a publicly available interface and providing publicly available constructor for internal *formatters.Storage.

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