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

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

phrfpeixoto
Copy link
Contributor

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
Contributor 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
Contributor Author

@stof Let me know what changes you would prefer.

@Seldaek
Copy link
Owner

Seldaek commented Oct 23, 2024

What's the use case here? Why is 'a' not good enough? Just curious/trying to understand. But in general I'd say given the edge case-ness here, I feel like a setter might be more appropriate than a constructor arg.

@phrfpeixoto
Copy link
Contributor Author

@Seldaek The use case is having the file overwritten, instead of appended.

@Seldaek Seldaek merged commit 8ae546b into Seldaek:2.x Nov 11, 2024
37 of 38 checks passed
@Seldaek
Copy link
Owner

Seldaek commented Nov 11, 2024

Thanks

@Seldaek Seldaek added this to the 3.x milestone Nov 11, 2024
@Seldaek Seldaek modified the milestones: 3.x, 2.x Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants