Page MenuHomeSoftware Heritage

npm.loader: Extract _author_str function and add tests around
ClosedPublic

Authored by ardumont on Wed, Jul 22, 3:19 PM.

Details

Summary

The goal is to fix an issue in a dedicated diff [1]

[1] D3596

Test Plan

tox

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Wed, Jul 22, 3:19 PM
ardumont updated this revision to Diff 12651.Wed, Jul 22, 3:22 PM

Simplify test

Build is green

Patch application report for D3595 (id=12650)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit db87f1d415fdec54607fa1db0040c496adf5cd04
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add tests around

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/185/ for more details.

ardumont updated this revision to Diff 12652.Wed, Jul 22, 3:25 PM

Read simplification without typos ;)

Build is green

Patch application report for D3595 (id=12651)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit 6050cdd4a63ffccf785113c7ff0bbf1ab9344bb4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add tests around

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/186/ for more details.

Build is green

Patch application report for D3595 (id=12652)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit fb26841643b5a32cdf8a5d16c896cf6218289ef5
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add tests around

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/187/ for more details.

ardumont updated this revision to Diff 12653.Wed, Jul 22, 3:41 PM

Add missing cases (except the problematic one for another diff)

Build is green

Patch application report for D3595 (id=12653)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit 1abbcfc1ff85b5696865e0a61962a71ebc3aa4ee
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add tests around

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/188/ for more details.

ardumont edited the summary of this revision. (Show Details)Wed, Jul 22, 3:46 PM
ardumont updated this revision to Diff 12655.Wed, Jul 22, 3:57 PM

Add types as well...

Build has FAILED

Patch application report for D3595 (id=12655)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit c4a4ddd37dd1d124d9b4c9f7de300fc0dd482d4e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add types and tests

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/190/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/190/console

Build has FAILED

Patch application report for D3595 (id=12655)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit c4a4ddd37dd1d124d9b4c9f7de300fc0dd482d4e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add types and tests

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/194/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/194/console

Build has FAILED

Picture me astonished ;)

ardumont added inline comments.Wed, Jul 22, 4:42 PM
swh/loader/package/npm/loader.py
190

yeah... that won't work ;)

ardumont updated this revision to Diff 12659.Wed, Jul 22, 4:42 PM

Fix typo again!

swh/loader/package/npm/loader.py
190

was "email" instead of "name", fixed.

Build is green

Patch application report for D3595 (id=12659)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit 033591dfc3aacb4cc5078fda3ab12170dc231c70
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add types and tests

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/195/ for more details.

I assume you changed the behavior of _author_str to deal with present by empty emails. If so, could you add a test for that?

vlorentz requested changes to this revision.Thu, Jul 23, 9:58 AM
This revision now requires changes to proceed.Thu, Jul 23, 9:58 AM

I assume you changed the behavior of _author_str to deal with present by empty emails. If so, could you add a test for that?

No.
Did i change the behavior here (I assumed i did not)?

Former code:

if "email" in author_data:
    author_str += " <%s>" % author_data["email"]

New code:

email = author_data.get("email")
if email is not None:
    author_str += f" <{email}>"

What is the benefit of the new code, if not to handle the case where author_data["email"] is None?

What is the benefit of the new code, if not to handle the case where author_data["email"] is None?

1 access less to author_data...
I did not realize the extra case, thanks.

This diff was just initially about extracting and starting having dedicated tests about it.
I started refactoring it in the next diffs and thought... well it can be done here instead... (to avoid mixing concerns).

I'll had the missing test case.
Thanks.

(fortunately, jenkins is back to "normal" speed now... so that should go fast)

ardumont updated this revision to Diff 12668.Thu, Jul 23, 10:18 AM

Add missing tests scenario

Build is green

Patch application report for D3595 (id=12668)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit 9894b477f5a226b8256f8a2eb56c6695883fdf12
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function + add types, tests

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/199/ for more details.

vlorentz accepted this revision.Thu, Jul 23, 10:28 AM
This revision is now accepted and ready to land.Thu, Jul 23, 10:28 AM