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

Update libphonenumber-csharp 8.13.34 #15669

Merged
merged 6 commits into from
Apr 8, 2024
Merged

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Apr 5, 2024

@hishamco hishamco requested a review from agriffard as a code owner April 5, 2024 17:41
Copy link
Contributor

github-actions bot commented Apr 5, 2024

This pull request has merge conflicts. Please resolve those before requesting a review.

@Piedone
Copy link
Member

Piedone commented Apr 5, 2024

Did you test phone number validation with the new version?

@hishamco hishamco requested review from Piedone and removed request for agriffard April 7, 2024 19:31
@MikeAlhayek
Copy link
Member

@hishamco why do we need a test case to validate an external library here? Part of the adopted library has it's own testing and we should not need to add a testing on the top of their testing.

@Piedone
Copy link
Member

Piedone commented Apr 8, 2024

I didn't mean that you should write a test. Rather, I'm asking whether you tested the phone number validation feature where it's used, like the user editor on the admin.

@hishamco
Copy link
Member Author

hishamco commented Apr 8, 2024

@hishamco why do we need a test case to validate an external library here? Part of the adopted library has it's own testing and we should not need to add a testing on the top of their testing.

Mike, I thought Zoltan wanted a unit test

I'm asking whether you tested the phone number validation feature where it's used, like the user editor on the admin.

I didn't because there's no major change in the library

@MikeAlhayek
Copy link
Member

Mike, I thought Zoltan wanted a unit test

I would remove it. Check out the last comment from Zoltan #15669 (comment)

@hishamco
Copy link
Member Author

hishamco commented Apr 8, 2024

Check out the last comment from Zoltan #15669 (comment)

I already replied :)

@MikeAlhayek MikeAlhayek merged commit e1bde3d into main Apr 8, 2024
5 checks passed
@MikeAlhayek MikeAlhayek deleted the hishamco/libphonenumber-csharp branch April 8, 2024 02:44
@Piedone
Copy link
Member

Piedone commented Apr 8, 2024

I'm just stating the obvious, but you should always test your code changes by running them, even if the change seems trivial, and especially if it's somewhat opaque like a dependency update. The reviewer has to test the changes, too.

@hishamco
Copy link
Member Author

hishamco commented Apr 8, 2024

The reviewer has to test the changes, too.

Frankly I never saw this previously in the packages update, unless it's a major release coz it might have a breaking changes

@Piedone
Copy link
Member

Piedone commented Apr 8, 2024

Every release can have breaking changes. We've seen breaking changes in .NET patch releases even. That it's not supposed to have them is no guarantee. So, naturally, you have to test the update. Flipping the version number in two files is not enough.

@hishamco
Copy link
Member Author

hishamco commented Apr 8, 2024

We've seen breaking changes in .NET patch releases even

You can blame them :) especially if they are using Semver

Flipping the version number in two files is not enough.

This is too much, coz you don't know what you want to test

@Piedone
Copy link
Member

Piedone commented Apr 8, 2024

That's part of the task: You need to check what's the impact of your change.

@hishamco
Copy link
Member Author

hishamco commented Apr 8, 2024

So I will leave updating the packages, I don't think @agriffard did that for every update in the past. It would be tedious

@Piedone
Copy link
Member

Piedone commented Apr 9, 2024

Well, is it really controversial that you have to test your changes? If a library update breaks something, what routinely happens (I personally found this on multiple occasions while reviewing, and thus also testing, PRs in OC), who has to discover that and fix it?

Keeping dependencies up-to-date is a valuable task. However, simply updating the version numbers is something that a script can do. The value lies in a human also determining that the change is safe. (And well, we could also have further automated testing, but human verification will still be necessary.)

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.

3 participants