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

Allow the user to define a custom mode when fopen-ing a log file #1913

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

phrfpeixoto
Copy link

I've included changes to both allow custom fopen modes, as well as change the visibility of the helper functions on this class.
I've made them final to keep the "do not touch" aspect, while still allowing subclasses to use them.

@stof
Copy link
Contributor

stof commented Sep 17, 2024

with the new argument, do you actually need to extend the class and call those protected functions ? If yes, why ? If no, I would not change their visibility as making them protected means they become part of the API that is covered by semver (and so they cannot be refactored to change their signature or be removed if the code does not need them anymore)

@phrfpeixoto
Copy link
Author

phrfpeixoto commented Sep 17, 2024

It doesn't. I've added that specifically to bring that discussion:

  • If I change the method visibility I can override the class and solve my problem, so I don't need the extra parameter.
  • If I add the extra parameter, I no longer need to override the class, so I don't need the method visibility.

Whichever change is less disruptive for the project is OK for me.

IMO, it's likely that you'll want to change the class constructor's signature (other than those helper functions) as time passes, so if you're worried about semver, it would be better to expose those helper methods (they are final, after all).

@phrfpeixoto
Copy link
Author

@stof Let me know what changes you would prefer.

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

Successfully merging this pull request may close these issues.

2 participants