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

Request: Move handlers into their own functions #101

Open
DrKain opened this issue Dec 7, 2023 · 8 comments
Open

Request: Move handlers into their own functions #101

DrKain opened this issue Dec 7, 2023 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed priority: medium This is something that should be done sooner rather than later

Comments

@DrKain
Copy link
Owner

DrKain commented Dec 7, 2023

Right now almost all of the work is done in a single function, clean().
I want to split each of the parts into their own functions for easier managing and more control over the process.

Currently this is what happens in the clean function (not in order):

  • Redirecting based on parameter
  • Rebuilding URL
  • Delete parameters
  • Updating path names
  • Handle AMP redirects
  • Fetching matching rules
  • Decode handler
  • Empty hashes
  • Removing empty values
  • Checking reduction/difference

I think handlers should get their own file to make the project easier to read and/or debug.
If anyone has thoughts on this feel free to contact me privately (email on my profile) or you can leave a comment on this issue.

@DrKain DrKain added enhancement New feature or request help wanted Extra attention is needed priority: medium This is something that should be done sooner rather than later labels Dec 7, 2023
@jugaltheshah
Copy link

I am interested in taking this on. I'm thinking (as a first pass at least) of splitting each of the aforementioned things into functions. Also, what are your thoughts on clean() taking a config object or something so that the caller can decide which parts of the cleaning process they want to run? I'm thinking object to allow for more flexibility & ease in adding/removing options down the line.

@DrKain
Copy link
Owner Author

DrKain commented Jan 4, 2024

That sounds good to me as long as the config object is optional. My original goal was simplicity, being able to pass any URL to be cleaned with ease, but I don't mind being able to customize the clean for more advanced users. So far the main options were for AMP links and redirects, as you've probably seen already

@apodi
Copy link

apodi commented Apr 8, 2024

I am interested. I will refactor the code and add some unit tests to make the method cleaner and more readable. Additionally, I will organize the handlers into different files.

@DrKain
Copy link
Owner Author

DrKain commented Apr 8, 2024

Thank you

@apodi
Copy link

apodi commented Apr 8, 2024

I think i do not have permission to push
git push --set-upstream origin refactor/handlers
remote: Permission to DrKain/tidy-url.git denied to apodi.
fatal: unable to access 'https://github.com/DrKain/tidy-url.git/': The requested URL returned error: 403
could you please add me as Collaborator

@DrKain
Copy link
Owner Author

DrKain commented Apr 8, 2024

Create a pull request. I'll review it when I get the time and if it's good I'll merge

@DrKain
Copy link
Owner Author

DrKain commented Apr 8, 2024

I'm not going to let you push code into the main branch that hasn't been tested, reviewed or even seen by me.

@apodi
Copy link

apodi commented Apr 8, 2024

PR is ready for review #108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed priority: medium This is something that should be done sooner rather than later
Projects
None yet
Development

No branches or pull requests

3 participants