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

LocaleCompat.forLanguageTag: return Optional if parsing fails #1151

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

Profpatsch
Copy link
Contributor

It’s not obvious that the function will fail in some cases and throw an IllegalArgumentException.

So instead of just failing if parsing fails, return an Optional that all callers have to decide what to do (e.g. the YoutubeExtractor can just ignore the locale in that case, like it does with most other fields in the json if they are unexpected).

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

Profpatsch and others added 2 commits January 7, 2024 14:31
It’s not obvious that the function will fail in some cases and throw
an `IllegalArgumentException`.

So instead of just failing if parsing fails, return an Optional that
all callers have to decide what to do (e.g. the YoutubeExtractor can
just ignore the locale in that case, like it does with most other
fields in the json if they are unexpected).
@Stypox Stypox force-pushed the localization-return-optional branch from eb52713 to 23fc7aa Compare March 29, 2024 12:45
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! I pushed a commit that uses ParsingException instead of IllegalArgumentException in places where the former makes more sense.

@Stypox Stypox merged commit e5b30ae into TeamNewPipe:dev Mar 29, 2024
4 checks passed
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