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

fix: Fix the default slug used for the package name #245

Merged
merged 5 commits into from
Mar 29, 2024

Conversation

abhijeetSaroha
Copy link
Contributor

@abhijeetSaroha abhijeetSaroha commented Mar 21, 2024

I added a function to sanitize the package_slug. So, now, It will remove the non-alphanumeric characters from the package_slug and will not give error.

Solves #197

@abhijeetSaroha abhijeetSaroha requested a review from xmnlab March 21, 2024 15:39
Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

Hey @abhijeetSaroha thanks for working on that.

I think that the only (or most only) place you need to change is here: https://github.com/osl-incubator/scicookie/blob/main/src/scicookie/profiles/base.yaml#L40

@abhijeetSaroha
Copy link
Contributor Author

@xmnlab
Here, I think only the default value can be managed. What if a user enters their own package_slug with unwanted symbols? How can that be handled here?

@xmnlab
Copy link
Member

xmnlab commented Mar 21, 2024

Yep the point is to not have a default value wrong. If the user decide any wrong value it should just fail.

Does it make sense?

@abhijeetSaroha
Copy link
Contributor Author

package_slug:
  message: Type the code name for your package (the name used to import your package)
  help: https://osl-incubator.github.io/scicookie/guide/#information-about-the-project
  type: text
  default: "{{project_slug.replace('-', '_').replace('.', '').replace('@', '').replace('&', '').replace('$', '').replace('%', '').replace('#', '')}}"
  visible: true

This is one of the way (works fine), but It doesn't seem to be good for production.

Then, I also tried regex, but I am not able to find Builtin Filter which has support of regex.

Then, I tried to build the logic which looks like this:

package_slug:
  message: Type the code name for your package (the name used to import your package)
  help: https://osl-incubator.github.io/scicookie/guide/#information-about-the-project
  type: text
  default: "{% set res='' %}{% for char in project_slug.replace('-','_') %}{% if char.isalnum() or char == '_' %}{% set res = res + char %}{% endif %}{% endfor %}{{ res }}"
  visible: true

But, after this, it gives me default value as empty string, if I give some initial value to the res, then default value would be the same. The for loop or the logic inside is not working.

Do you have any idea more about this?

@xmnlab
Copy link
Member

xmnlab commented Mar 21, 2024

I think that the way to go using jinja2 filters.

so you could create a more robust regex here: https://github.com/osl-incubator/scicookie/blob/main/src/scicookie/ui.py

Here is an example of implementation (pseudo code) and how to use it:

import re
from jinja2 import Environment

# Define the custom filter
def sanitize_package_slug(package_slug: str):
    """A custom Jinja2 filter to perform regex replacement."""
    #  do you trick here
    return sanitized_string

# Create a Jinja2 environment and add the custom filter
env = Environment()
env.filters['sanitize_package_slug'] = sanitize_package_slug

# Now you can use this filter in your templates
template_string = """
default: "{{project_slug | sanitize_package_slug }}"
"""

in the original code we are using Template, so you will need to change it to Environment (as the same in the example)

@abhijeetSaroha
Copy link
Contributor Author

@xmnlab , can you please review the changes, on my side I test the changes and works fine.

@abhijeetSaroha abhijeetSaroha requested a review from xmnlab March 28, 2024 18:27
Copy link
Member

@xmnlab xmnlab 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 working on that @abhijeetSaroha
just added a few notes here.

@@ -3,13 +3,14 @@
from __future__ import annotations

import os
import re # Add this import for regex
Copy link
Member

Choose a reason for hiding this comment

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

you don't need this comment here .. it is really clear that re is used for regex among the python users
so no worries about the comment

@@ -96,11 +104,17 @@ def make_questions(questions: dict[str, Any]) -> dict[str, str]:
)
print("." * columns)

# Create a Jinja2 environment and add the custom filter
env = Environment(autoescape=True)
Copy link
Member

Choose a reason for hiding this comment

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

please use autoescape=false.

if bandit complains, just skip the issue there, for example: https://github.com/osl-incubator/makim/blob/main/pyproject.toml#L105

@abhijeetSaroha
Copy link
Contributor Author

I removed the comment and autoescape also, so it is now false by default.

@xmnlab xmnlab changed the title fix: slug name fix: Fix the default slug used for the package name Mar 29, 2024
@xmnlab xmnlab merged commit 9c53ee3 into osl-incubator:main Mar 29, 2024
12 checks passed
@xmnlab
Copy link
Member

xmnlab commented Mar 29, 2024

thanks @abhijeetSaroha

Copy link

github-actions bot commented May 3, 2024

🎉 This PR is included in version 0.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants