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

Reimplement Slugify #7461

Closed
wants to merge 2 commits into from
Closed

Reimplement Slugify #7461

wants to merge 2 commits into from

Conversation

ahmedhosssam
Copy link
Contributor

This PR addresses the issue #7450

Note: I know there is another open PR that solves the same issue, but I'm trying to make any code changes to gain more understanding of the code. (I'm preparing myself for gsoc, so I'm not waiting for a merge more than feedback)

PR Description:

I think that the bug was having more than one dot in some files and changing it results some confusion, so I think this implementation solves the problem.
But there is one failed test that checks for file.greg.fits to be file_greg.fits
Should we change the test? Or should we stick with the original function?

@nabobalis
Copy link
Contributor

nabobalis commented Feb 28, 2024

Hello @ahmedhosssam, as you have already said, there is already a PR opened to address this here: #7453 and it works by redoing how we normalize the filenames. I would recommend looking at how the code was changed in that pull request to understand the other ways this can be handled.

I do not want the limited time our reviewers have to review PRs that will not be merged in.
I hope you understand.

@nabobalis nabobalis closed this Feb 28, 2024
@ahmedhosssam ahmedhosssam deleted the test branch May 22, 2024 09:54
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.

None yet

2 participants