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

Add getters to FFmpegOutputBuilder and deprecate property access #320

Conversation

Euklios
Copy link
Collaborator

@Euklios Euklios commented Mar 13, 2024

The goal is to allow changes to classes' internal structure and storage layout without affecting their public interface. Getters can be added to return the same information as before, while the internal might keep its information in a different way. I plan to use this in #318 to add details about each option. The getters allow me to create a seamless transition between old and new. Additionally, this makes the API more consistent within the library and other Java code.

@Euklios Euklios assigned Euklios and bramp and unassigned Euklios Mar 13, 2024
@bramp
Copy link
Owner

bramp commented Mar 13, 2024

and sorry can you briefly describe why you are making this change. What benefit does it have?

@Euklios
Copy link
Collaborator Author

Euklios commented Mar 16, 2024

and sorry can you briefly describe why you are making this change. What benefit does it have?

The goal is to allow changes to classes' internal structure and storage layout without affecting their public interface. Getters can be added to return the same information as before, while the internal might keep its information in a different way. I plan to use this in #318 to add details about each option. The getters allow me to create a seamless transition between old and new. Additionally, this makes the API more consistent within the library and other Java code.

@Euklios Euklios requested a review from bramp March 16, 2024 08:33
Copy link
Owner

@bramp bramp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, my only concern is if this is worth it for the simple "Immutable" objects such as Format?, e.g

@Immutable
public class Format {
  final String name;
  final String longName;
...

They represent a data structure, that isn't going to change (or at least I don't see how it would right now).

But I won't block you from making changes... Either way this seems a API change, so we should bump the version number appropriately.

@Euklios Euklios force-pushed the feature/deprecate-public-fields-in-OutputStreamBuilder branch from 36826ab to de4b288 Compare March 20, 2024 14:29
@Euklios
Copy link
Collaborator Author

Euklios commented Mar 20, 2024

@bramp Point taken. I've reverted the deprecations on info and progress objects and kept them for the builders and options.
Additionally, I kept the getters on info and progress objects as an add-on for people who prefer them or if required by frameworks.

As for versions: There shouldn't be a change impacting users (apart from deprecation notices ofc.). I'll let you decide what impact on the version number that has

@Euklios Euklios merged commit faedc82 into bramp:master Mar 20, 2024
4 checks passed
@Euklios Euklios deleted the feature/deprecate-public-fields-in-OutputStreamBuilder branch March 20, 2024 14:47
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