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

logger::fatal(...) terminates the program - investigate alternatives #219

Open
3 tasks done
AlexRamRam opened this issue Jun 5, 2023 · 2 comments
Open
3 tasks done
Labels
difficulty/01-good-first-issue Very approachable for new contributors enhancement/refactor Improves flexibility/maintainability

Comments

@AlexRamRam
Copy link
Contributor

Affected Branch

trunk

Basic Diagnostics

  • I've pulled the latest changes on the affected branch and the issue is still present.

  • The issue is reproducible in docker

Description

It's surprising to a reader that the logger object has the power to terminate the program when fatal() is called.

Consider these alternatives to improve code clarity:

  • remove the exit() call from logger::fatal() and client to invoke exit(1)
  • take a logger::fatal) takes additional exit code parameter
  • rename logger::fatal() to panic() or fatal_system_exit()

Code of Conduct

  • I agree to follow this project's Code of Conduct
@AlexRamRam AlexRamRam added the fix/bug Fixes errant behavior label Jun 5, 2023
@HalosGhost HalosGhost added enhancement/refactor Improves flexibility/maintainability difficulty/01-good-first-issue Very approachable for new contributors and removed fix/bug Fixes errant behavior labels Jun 5, 2023
@HalosGhost HalosGhost removed their assignment Jun 5, 2023
@AlexRamRam
Copy link
Contributor Author

Use of the "fatal" interface for a logging mechanism (to log then system exist) may be a common convention. Golang is one (significant) example:

From a personal experience, I do think something like fatal_system_exit() would provide better clarity. However, there is a strong argument to close this issue item if fatal is a well-known convention.

@HalosGhost I will leave the decision of closing this item up to you.

@HalosGhost
Copy link
Collaborator

HalosGhost commented Oct 16, 2023

It's not an unheard-of idiom. But, I've never found it well-structured from a separation-of-concerns perspective (seems odd to me that the logger should really be able to control whether execution continues). I'm not particularly certain there's a better plan without rearchitecting a lot of code (this is one of those small implementation detail decisions that has far-reaching effects if you change it because it violates basic separation-of-concerns).

So, I'm comfortable with this being closed, being fixed by a rename, or proposals for something better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty/01-good-first-issue Very approachable for new contributors enhancement/refactor Improves flexibility/maintainability
Projects
None yet
Development

No branches or pull requests

2 participants