Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixed epoch parser failing for numeric values #3520

Merged

Conversation

OmkarPh
Copy link
Contributor

@OmkarPh OmkarPh commented Sep 24, 2023

Fixes #3489

Tasks

  • Reviewed contribution guidelines
  • PR is descriptively titled 📑 and links the original issue above 🔗
  • Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
    Run tests locally to check for errors.
  • Commits are in uniquely-named feature branch and has no merge conflicts 📁
  • Looked for possible updates in documentation and added updates if applicable
  • Updated CHANGELOG.rst

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @OmkarPh
See a few nits for your consideration

@@ -391,3 +391,7 @@ def test_rpm_tags_xsetup_0_28_b1_src_rpm(self):
def test_rpm_tags_zziplib_0_11_15_3sf_i586_rpm(self):
test_file = self.get_test_loc('rpm/header/zziplib-0.11.15-3sf.i586.rpm')
self.check_rpm_tags(test_file)

def test_rpm_tags_apache_commons_io_2_4_12_el7_noarch_rpm(self):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe test_get_rpm_tags_can_parse_numeric_values is a better name?

Copy link
Contributor Author

@OmkarPh OmkarPh Sep 25, 2023

Choose a reason for hiding this comment

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

the tests above also check for the same function right
Should I update those too?

@pombredanne
Copy link
Member

You can use this truncated RPM that I shortened using an hex binary editor (ghex)
apache-commons-io-2.4-12.el7.noarch.rpm.zip

Signed-off-by: Omkar Phansopkar <omkarphansopkar@gmail.com>
@OmkarPh OmkarPh force-pushed the fix/rpm-epoch-error branch from 3d5f7ec to 2405a89 Compare September 25, 2023 17:04
@OmkarPh
Copy link
Contributor Author

OmkarPh commented Sep 25, 2023

You can use this truncated RPM that I shortened using an hex binary editor (ghex) apache-commons-io-2.4-12.el7.noarch.rpm.zip

Thanks, I've force pushed (and tested) with this truncated file

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks++ @OmkarPh
Merging!

@AyanSinhaMahapatra AyanSinhaMahapatra merged commit c84c8e9 into aboutcode-org:develop Oct 10, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to scan RPM
3 participants