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

fix: fix pricePerBroadcaster JSON file line breaks parsing bug #2913

Closed

Conversation

rickstaa
Copy link
Member

@rickstaa rickstaa commented Dec 5, 2023

This pull request ensures that the pricePerBroadcaster flag also correctly parses JSON files when they contain line breaks (see #2912).

What does this pull request do? Explain your changes. (required)

The current go-livepeer livepeer binary does not correctly parse pricePerBroadcaster files if they contain line breaks.

This pull request ensures that the pricePerBroadcaster flag also works with JSON files that contain line breaks as is often done to improve readability.

Specific updates (required)

  • The common.ReadFromFile was changed so JSON files with line breaks and JSON strings are parsed correctly.

How did you test each of these updates (required)

I debugged the changed code with both the JSON file and JSON string format in Vscode.

Does this pull request close any open issues?

Fixes #2912.

Checklist:

This commit ensures that the `pricePerBroadcaster` flag also correctly
parses JSON files when they contain line breaks (see livepeer#2912).
@rickstaa rickstaa changed the title fix: fix 'pricePerBroadcaster' file parsing bug fix: fix pricePerBroadcaster file parsing bug Dec 5, 2023
@rickstaa
Copy link
Member Author

rickstaa commented Dec 5, 2023

The test failed because the behaviour was changed. We should discuss whether this change is good since I do not know why only the first line was returned in the old code. I can update the tests if the new behaviour is the desired behaviour 👍🏻 .

@rickstaa rickstaa changed the title fix: fix pricePerBroadcaster file parsing bug fix: fix pricePerBroadcaster JSON file line breaks parsing bug Dec 5, 2023
@thomshutt
Copy link
Contributor

@rickstaa looks good minus some newline trimming and test updates - I went ahead and did them in #2914 to speed things up, but will leave your name in the Changelog!

@thomshutt thomshutt closed this Dec 7, 2023
@rickstaa
Copy link
Member Author

rickstaa commented Dec 7, 2023

@rickstaa looks good minus some newline trimming and test updates - I went ahead and did them in #2914 to speed things up, but will leave your name in the Changelog!

Hello @thomshutt,

I appreciate your prompt review of my pull request ❤️‍🔥. Your observations were correct; indeed, the tests and line trimming still needed to be improved (see #2913 (comment)) 👍🏻. Just a quick tip: Git allows you to add people as co-authors to commits. I use this method in most of my OS projects to include people's contributions into my branch while not having to squash or cherry-pick their commits. This promotes a collaborative environment while keeping the commit history clean. No problem with this PR since it was a 2-minute fix 🤣, but it is worth considering for future cases.

@rickstaa rickstaa deleted the fix_pricePerBroadcaster_parse_bug branch December 7, 2023 14:38
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.

pricePerBroadcaster argument doesn't work JSON files that contain line-breaks
2 participants