backintime icon indicating copy to clipboard operation
backintime copied to clipboard

Published 20 hours ago • verified publisher icon bit-team

Replace isipv6 method with builtin package

Open jarbhav opened this issue 10 months ago • 10 comments

Closes #1686

  • Removed isIPv6Address() function and related tests
  • Using ipaddress package directly in the escapeIPv6Address function

jarbhav avatar Apr 15 '24 06:04 jarbhav

Dear Vaibhav, Thank you for your contribution. I appreciate it.

Looks nice. Good work. I added some review comments to the code.

Beside my comments it would be nice if you could improve tests. The behavior of the function escapeIPv6Address() has three outcomes: 1) escaped IP 2) not escaped IP 3) raised ValueError. This three outcomes could be covered in three different unit tests. The current test do cover only 1) and 2) in one function but it would improve test quality if each of the three outcomes are tested in separate test methods.

Let me know if I can be of assistance.

Best, Christian

buhtz avatar Apr 15 '24 07:04 buhtz

Hey Christian, I am unable to see the review comments. I will get started on the test cases.

jarbhav avatar Apr 15 '24 16:04 jarbhav

Dear Vaibhav,

On 2024-04-15 09:19 Vaibhav Raj @.***> wrote:

Hey Christian, I am unable to see the review comments. I will get started on the test cases.

I wonder how this could be. Do you read via email or do you visit github.com? When you visit github.com and visit your on PR you will see something like this (see attached screenshot). I added two circles with numbers to the picture to point you to the comments.

Can't you see this on the real github.com website?

Kind Christian

buhtz avatar Apr 15 '24 16:04 buhtz

image

jarbhav avatar Apr 15 '24 17:04 jarbhav

Your screenshot looks like the "Files changed" section. My apologize. I think the term I used (code comments) is not correct. Correct would be review comments. Have a look at this screencast. Does this help you? Peek 2024-04-15 20-02

But you also should see this review comments in the "Files changed" section when scrolling down. grafik

buhtz avatar Apr 15 '24 18:04 buhtz

Dear Vaibhav, can I somehow be of assistance? Kind, Christian

buhtz avatar Apr 23 '24 07:04 buhtz

Dear Vaibhav, can you please give us some feedback about your plans and your next steps. Best, Christian

buhtz avatar May 11 '24 12:05 buhtz

Hey Christian, I apologize for the lack of response; I will try to get back to you with all the changes above in the coming 3 days. Please let me know if this should be done sooner. Vaibhav

jarbhav avatar May 12 '24 14:05 jarbhav

Dear Vaibhav, Thank you for the reply.

I will try to get back to you with all the changes above in the coming 3 days. Please let me know if this should be done sooner.

That is totally fine for me. Take your time. No need to hurry. I just need to know your schedule to "integrate" your PR into the rest of our workflow and tasks. Sometimes we have contributors throwing in a PR but not finalizing it.

Best, Christian

buhtz avatar May 12 '24 15:05 buhtz

Dear Christian, I have made the suggested changes, also separated the test method into three different methods. Please let me know if any changes are required

Vaibhav

jarbhav avatar May 13 '24 19:05 jarbhav

Dear Vaibhav, please apologize my late reply. Your modifications looking good. I just did some refactoring and thing we are now ready to merge. I will merge in some days after a cool down period. Please see What happens after you opened a Pull Request (PR)? about how we handle this usually.

  • I refactored the unit tests a bit.
  • I rewinded my ValueError proposal. Other tests failed because they where using host names (e.g. "localhost").

buhtz avatar May 21 '24 10:05 buhtz

Thanks Christian

jarbhav avatar May 21 '24 10:05 jarbhav