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

Report duplicate names via printer #336

Closed
auduchinok opened this issue Aug 20, 2019 · 3 comments · Fixed by #339
Closed

Report duplicate names via printer #336

auduchinok opened this issue Aug 20, 2019 · 3 comments · Fixed by #339
Labels
Console work easy Bugs or features that are easy to solve for a beginner. enhancement help wanted

Comments

@auduchinok
Copy link
Contributor

auduchinok commented Aug 20, 2019

Could it be better for tooling if the error about duplicate names was reported via relevant printer instead of being written to the output? exn and info printers seem to suite well enough.

Although it would probably be a better design, the tooling would have to still support current approach as some existing projects may not get an updated Expecto package anytime soon. Due to this I'm not sure if it'd be a better overall situation for the tools but just wanted to share the thoughts.

@haf
Copy link
Owner

haf commented Aug 20, 2019

@auduchinok Yes indeed. It's probably just an oversight.

Change logger.logWithError (...) to config.printers.info (...) on this line: https://github.com/haf/expecto/blob/master/Expecto/Expecto.fs#L1953 — that way tooling can inject its own loggers.

This is a perfect getting-started issue!

@haf haf added Console work easy Bugs or features that are easy to solve for a beginner. enhancement help wanted labels Aug 20, 2019
@auduchinok
Copy link
Contributor Author

Are you sure about exn? It takes both a string and an exception which means we'd have to create some artificial exception probably duplicating the string. It also takes a time span which we don't have, though it could be just 0. info seems to suit better given parameter types it expects.

If sticking with exn, should a separate exception class be added or TestDiscoveryException is OK?

@haf
Copy link
Owner

haf commented Aug 20, 2019

Sure, info — nice catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Console work easy Bugs or features that are easy to solve for a beginner. enhancement help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants