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

Regex support added for ignore URL list in settings.py #116

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sauravsharma1998
Copy link

Regex Support Added for IGNORE_URLS:
admin/* [All URL pattern with starting with admin/ will be ignored for GUID generation]

Suggestion Link: #104

@sauravsharma1998
Copy link
Author

Can someone please review the installation failure for Django 5.0
image

@sauravsharma1998 sauravsharma1998 marked this pull request as draft February 14, 2024 13:05
@sauravsharma1998
Copy link
Author

sauravsharma1998 commented Feb 15, 2024

@ingvaldlorentzen @JonasKs @nschlemm Can you please review workflows for Django Version 5.0 above.

@JonasKs
Copy link
Member

JonasKs commented Feb 15, 2024

Thanks for the PR!
Don't worry about the pipelines, I would look into them before merging.

The code needs tests, and I'd like to use a re.compile solution instead of this. Inspiration can be found here, and the check can be found in the same file, on line 45. 😊

@sauravsharma1998
Copy link
Author

@JonasKs Thanks for your recommendations.

Followed this - The code needs tests, and I'd like to use a re.compile solution instead of this. Inspiration can be found here

Updates:

  1. Fixed one previous test case where monkey patching was missing for updating the django settings
  2. Added the test case for regex support for IGNORE_URL

Please have a look on the Django 5.0 workflows.

@sauravsharma1998
Copy link
Author

@JonasKs @nschlemm @ingvaldlorentzen Can you please review the PR.

@JonasKs
Copy link
Member

JonasKs commented Feb 16, 2024

I'll get to it when I have time. I won't have time until next week at earliest.
This isn't paid work, it's done in our spare time.

@sauravsharma1998
Copy link
Author

@JonasKs No issues. Check this as per your convenience. Thanks 👍

@sauravsharma1998 sauravsharma1998 marked this pull request as ready for review February 16, 2024 18:00
@JonasKs JonasKs closed this Apr 25, 2024
@JonasKs
Copy link
Member

JonasKs commented May 29, 2024

Wow, I definitely didn't intend to just close this out of nowhere. I was looking for this now, and couldn't find it.

@sauravsharma1998 , I'm sorry.

@JonasKs JonasKs reopened this May 29, 2024
@JonasKs
Copy link
Member

JonasKs commented May 29, 2024

The implementation would be a breaking change - since you change the current ignore_urls behavior. This should be a completely new setting.

IGNORE_URLS = []
for url in settings.ignore_urls:
url_regex = url.replace('*', r'[\s\S]*') # noqa
url_regex = '^' + url_regex + '$'
Copy link
Member

Choose a reason for hiding this comment

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

If we make this a new setting, e.g. ignore_url_regex, we can also skip this.

Copy link
Author

Choose a reason for hiding this comment

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

@JonasKs Thanks for the suggestions. I have moved it to a new settings. But facing errors in CI pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

I’m on vacation, but I’ll check when I’m back. 😊

@JonasKs
Copy link
Member

JonasKs commented Jul 11, 2024

There's failing pipelines, even though I haven't changed anything and I've deleted all caches.. I'll have to look further into this, but I'm not sure what's happening.

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.

2 participants