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

Deb822 prologue: refactorings #203

Merged
merged 7 commits into from Jul 13, 2023
Merged

Deb822 prologue: refactorings #203

merged 7 commits into from Jul 13, 2023

Conversation

schopin-pro
Copy link
Contributor

@schopin-pro schopin-pro commented Jul 12, 2023

This PR contains various refactoring that will make it much easier to implement support for the APT deb822 source format.

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #203 (57ea4c2) into main (b4e81f4) will increase coverage by 0.03%.
The diff coverage is 92.09%.

@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
+ Coverage   82.66%   82.69%   +0.03%     
==========================================
  Files          91       91              
  Lines       18526    18558      +32     
==========================================
+ Hits        15314    15347      +33     
+ Misses       3212     3211       -1     
Impacted Files Coverage Δ
apport/packaging_impl/apt_dpkg.py 83.93% <88.39%> (+0.51%) ⬆️
tests/system/test_packaging_apt_dpkg.py 98.63% <98.41%> (-0.20%) ⬇️
tests/integration/test_packaging_apt_dpkg.py 100.00% <100.00%> (ø)

@schopin-pro schopin-pro changed the title Deb822 1 Deb822 prologue: refactorings Jul 12, 2023
@schopin-pro schopin-pro requested a review from bdrung July 12, 2023 15:30
tests/system/test_packaging_apt_dpkg.py Show resolved Hide resolved
tests/system/test_packaging_apt_dpkg.py Outdated Show resolved Hide resolved
tests/system/test_packaging_apt_dpkg.py Outdated Show resolved Hide resolved
tests/system/test_packaging_apt_dpkg.py Show resolved Hide resolved
tests/system/test_packaging_apt_dpkg.py Show resolved Hide resolved
Copy link
Collaborator

@bdrung bdrung left a comment

Choose a reason for hiding this comment

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

I finished the review of all commits. Except for some minor comment, the change looks good. I would love to see moving to pathlib (to make the code shorter and easier to read), but this is something for the future.

apport/packaging_impl/apt_dpkg.py Outdated Show resolved Hide resolved
apport/packaging_impl/apt_dpkg.py Show resolved Hide resolved
apport/packaging_impl/apt_dpkg.py Show resolved Hide resolved
apport/packaging_impl/apt_dpkg.py Outdated Show resolved Hide resolved
apport/packaging_impl/apt_dpkg.py Show resolved Hide resolved
apport/packaging_impl/apt_dpkg.py Outdated Show resolved Hide resolved
This centralizes where we write single-line APT sources files in the
tests.
We only implement the one-line style for now, but deb822 is coming.
…stic

This commit splits out the gathering of the data from the actual
heuristic implementation (which could hardly be simpler)
In the brave new world of deb822, there might not *be* a source.list
file that we can rely on!!

Note that this drops an early check for the existence of the source.list
file. That check was of fairly low value in terms of validating input in
the first place (source.list could be empty or full of garbage), and
replacing it with the apt dir would even lower that value, as the dir
itself could be empty. In addition to that, the exception raised is too
generic to be truly useful.
For such a tiny string, it's surprisingly complicated to find out which
are its components!

This allows us to drastically reduce the complexity of
create_ppa_source_from_origin()
This part is now self contained. Note that the new code is designed for
future extension when the deb822 format support will be implemented.
…string

This will make things easier when we switch to produce deb822 sources
down the road, and makes the code in the main function a bit more
readable.
@bdrung bdrung merged commit f4adbae into canonical:main Jul 13, 2023
24 checks passed
@schopin-pro schopin-pro deleted the deb822-1 branch February 13, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants