search

fangfufu / httpdirfs

A filesystem which allows you to mount HTTP directory listings or a single file, with a permanent cache. Now with Airsonic / Subsonic support!
Other
778 stars 59 forks source link

Handle sites that use absolute links and sites that require the final slash in the URL #121

Closed jikamens closed 1 year ago

jikamens commented 1 year ago

lol I see somebody else already encountered this #115. I guess I should have checked for PRs first. Anyway I'll leave it to you to decide which of these PRs to prefer. ;-)

jikamens commented 1 year ago

I will say that I think my version is more robust and better documented.

jikamens commented 1 year ago

OK, there's another bug fix for you in the last commit, plus a couple ancillary commits that aren't strictly speaking necessary for the bug fixes but I think are a good idea. :shrug:

jcharaoui commented 1 year ago

I would suggest to open separate merge requests for the different proposed changes. This PR is getting a little out of hand.

jikamens commented 1 year ago

I am happy to do that if someone is actually going to look at the changes but considering that I opened the PR weeks ago and there has been no response I don't know if that is going to happen. If your comment is intended to convey, "I have commit rights to this repository and if you open separate PRs for these changes then I will look at them," then I will do that. If you are just making a suggestion as an observer then I think I will hold off on doing the extra work to split the PR until I actually here from someone who has the access to review the changes and is willing to consider merging them.

jikamens commented 1 year ago

Also, most of the changes in the PR before the autotools changes I pushed today are intended for essentially the same purpose, i.e., getting this tool to work on sites where it doesn't currently, and therefore I think it's reasonable to review them together.

fangfufu commented 1 year ago

I am happy to do that if someone is actually going to look at the changes but considering that I opened the PR weeks ago and there has been no response I don't know if that is going to happen.

Sorry I had been a bit busy with my personal life. I just hadn't got time to look at it.

fangfufu commented 1 year ago

@jikamens, I agree with what @jcharaoui said.

Also, you seem to be very good at software engineering. I would be super grateful if you write some sort of test system for this. If you write a test system to prove that all your changes work, I will approve your pull request.

Right now I just have to manually test things. This really isn't ideal. I started this project before I became a professional software engineer. This sort of testing wouldn't fly in my company...

I am happy to approve your autotool related changes, if they were in a different pull request.

jikamens commented 1 year ago

Hi @fangfufu,

A few things:

1) I did not mean to imply any sort of judgment in my comment above about your not responding to the PR. I sincerely apologize if it came across that way. I understand that people have lives outside of their open-source projects and may not be able to respond immediately to PRs. It's just that from the outside there's no way to distinguish between that and someone who has decided to stop working on a project, and I am not inclined to spend additional time working on a project that isn't being actively maintained.

2) Having said that, I don't have capacity to build a unit-test system from scratch for someone else's project. If there were existing unit tests in this project then I would be happy to add tests for my changes, but I think asking a project contributor to create a unit-test system from scratch as the price of accepting their bug-fixes into your project is a bit much. The fact of the matter is that my changes clearly fix bugs -- I even indicated in the commits the sites you could use to test that the bug exist without my changes and go away with them -- and that I submitted them to the project for your benefit, not mine. I can always just keep using my fork without anyone else benefiting from my changes. :shrug: That's your call.

3) I just removed my most recent commit from the PR because you and @jcharaoui are correct that it muddies the waters and I don't want it to be a blocker to getting the real bug fixes in. I modified the build to use autotools because that makes it easier to build and package, so if you're interested in that change I can submit a separate PR for it, but I think it would be better to leave that aside for the time being and focus on the commits that are actual bug fixes.

fangfufu commented 1 year ago

@jikamens , I am interested in the autotool changes. Please submit a separate commit for it.

fangfufu commented 1 year ago

@jikamens

but I think asking a project contributor to create a unit-test system from scratch as the price of accepting their bug-fixes into your project is a bit much.

I didn't imply in any way that you have to create unit-tests to get these accepted. I was just saying that it would be a good contribution to do. I might just create an issue for it.

fangfufu commented 1 year ago

@jikamens Created pinned issue https://github.com/fangfufu/httpdirfs/issues/122 for the unit test framework.

fangfufu commented 1 year ago

@jikamens

I modified the build to use autotools because that makes it easier to build and package, so if you're interested in that change I can submit a separate PR for it, but I think it would be better to leave that aside for the time being and focus on the commits that are actual bug fixes.

Autotool changes can be done simultaneously as your other bug fixes. Once again, thanks for your contribution. I think autotool changes would be good for this project.

jikamens commented 1 year ago

See #123.