search

gardenshade / mam-plus

Tweaks & functionality for MAM
83 stars 6 forks source link

Upload search #161

Closed moriakaice closed 3 years ago

moriakaice commented 3 years ago

Added: Upload page support New feature: allow easy searching for duplicates when uploading new content

gardenshade commented 3 years ago

The only issue I found was that you must add an array of valid pages (usually just one) to the feature constructor to specify which pages it will run on. Leaving the array empty marks it as a global feature. I also bumped the version number down to be a patch instead of minor to be in line with previous updates.

moriakaice commented 3 years ago

@gardenshade Sorry, I've missed the part about adding an array of valid pages to the feature!

As for the patch vs minor: I've followed semver, but I understand that you version MAM+ a bit differently.

moriakaice commented 3 years ago

Also sorry for wrong target to merge, I'm so used to work with single long living branch now!

gardenshade commented 3 years ago

The branch issue was at least partially my fault because there was no dev branch uploaded at the time of your PR... for some reason it got deleted after the last release.

Unfortunately, this is my first collaborative repo and I didn't really know what I was doing when I started it (and still barely do) so nothing about MAM+ is Github-standard. I've considered switching to semver but it feels weird to do so in the middle of a project. I might switch at v5. I do like having the separate dev branch though, as it allows people the option to beta-test the script (though usually not far in advance) and I can verify that everything works as intended + update the changelog before it's released into the wild.

For instance, your feature would have been completely broken as-released because I hadn't noticed at first glance that the upload page selection would never trigger (it would have worked when it was trying to run on every page, but switching to upload broke it). This is because upload is actually tor/upload and needed to be a subpage check inside tor. (Moral of the story, don't make untested commits on PRs? Oops) The page checking logic was just changed before your PR so I hadn't got the documentation adjusted for that yet.

Either way, thanks so much for contributing! I had always hoped this would be a collaborative effort, but the first few years were just me, doing whatever, and now there's almost a handful of contributors. :)

moriakaice commented 3 years ago

I've tested only my changes in the context of the upload page (and I was convinced it's enough for the lack of ID for the selector would be enough). Maybe the learning is that there should be tests created to cover those?

Anyway, it was fun to contribute (even if the contribution had its own problems)! Thanks for putting in the effort to create something for others!