Skip to content

Slugify helper function should replace hyphens (bugfix)#1754

Merged
Hook25 merged 4 commits intomainfrom
modify-slugify
Feb 26, 2025
Merged

Slugify helper function should replace hyphens (bugfix)#1754
Hook25 merged 4 commits intomainfrom
modify-slugify

Conversation

@pieqq
Copy link
Copy Markdown
Collaborator

@pieqq pieqq commented Feb 26, 2025

Description

The slugify helper function is used in resource jobs for attributes that are then used for some job requirements. These attributes should be Python-compatible, meaning they cannot include hyphens.

Modifying the slugify function to replace hyphens with an underscore.

With this patch, strings like usb-vendor will be replaced with usb_vendor, making it a valid Python attribute.

Resolved issues

Fix #1745

Documentation

Tests

The slugify helper function is used in resource jobs for attributes that
are then used for some job requirements. These attributes should be
Python-compatible, meaning they cannot include hyphens.

Modifying the slugify function to replace hyphens with an underscore.

Fix #1745
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.32%. Comparing base (ef713cf) to head (8798d43).
Report is 128 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1754   +/-   ##
=======================================
  Coverage   49.31%   49.32%           
=======================================
  Files         374      374           
  Lines       40465    40467    +2     
  Branches     6835     6836    +1     
=======================================
+ Hits        19957    19959    +2     
  Misses      19783    19783           
  Partials      725      725           
Flag Coverage Δ
checkbox-support 62.00% <100.00%> (+62.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

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

small nitpick, if the string starts with a number, put an underscore in front!

@fernando79513
Copy link
Copy Markdown
Collaborator

Since we are also accepting dots, and that could change the "meaning" of the variable (1 variable vs variable.attribute)
I would also make that very clear in the function description, and maybe also add a simple test that include dots

@pieqq pieqq requested a review from Hook25 February 26, 2025 12:41
Hook25
Hook25 previously approved these changes Feb 26, 2025
Copy link
Copy Markdown
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

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

sorry for the pedantry

@Hook25 Hook25 merged commit 5618016 into main Feb 26, 2025
18 checks passed
@Hook25 Hook25 deleted the modify-slugify branch February 26, 2025 13:25
stanley31huang pushed a commit that referenced this pull request Mar 28, 2025
* Slugify helper function should replace hyphens

The slugify helper function is used in resource jobs for attributes that
are then used for some job requirements. These attributes should be
Python-compatible, meaning they cannot include hyphens.

Modifying the slugify function to replace hyphens with an underscore.

Fix #1745

* Slugify dots as well

* Prefix string with underscore if it starts with a digit

* Better digit check
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.

slugify() in snapd_resource.py breaks checkbox-newparis slot-check tests

3 participants