Hi there,

I really like your module, I've experimented with similar things and I'm really happy that you already Implemented my thoughts ;D

Here are some changes I need for my current project and may be good for others too:
- I don't want the ETA message; added an option to hide it
- Your not deleting used variables, but as d7 don't knows what variable belongs to which module, they have to be removed by hook_uninstall()
- Added some comments (where missing), removed trailing spaces, .. the general code review / best practices stuff (as far I know them)

Hope the patch is okay so far and if interested I'd also do some further development on your module as co-maintainer (flexibility/best practice).

ty

CommentFileSizeAuthor
#3 optional_eta_uninstall_variables-1347688-3.patch13.51 KBpatrickd
optional_eta_uninstall_variables--0.patch13.5 KBpatrickd

Comments

gielfeldt’s picture

gielfeldt commented

Hi Patrick

Thanks for the nice words.

I can see you've accidentally called the hook_uninstall() function in background_process.install for "background_batch_uninstall()". Copy/paste type I assume :-) Otherwise it looks great.

I'll grant you git access right away, then you can apply the patch yourself. You're now officially a co-maintainer :-)

gielfeldt’s picture

gielfeldt commented
Assigned: Unassigned » patrickd
Status: Needs review » Needs work
patrickd’s picture

patrickd commented
Status: Needs work » Needs review
StatusFileSize
new optional_eta_uninstall_variables-1347688-3.patch13.51 KB

aww, busted ^^ I corrected it directly but didn't redo the patch, sry

cool, thank you!
First project I'm co-maintaining hope I'll do it all right, otherwise please just hit me, I'm still learning ;)

gielfeldt’s picture

gielfeldt commented
Status: Needs review » Reviewed & tested by the community

Seems to work as expected. Ok by me. Thumbs up. Go ahead and apply.

patrickd’s picture

patrickd commented
Status: Reviewed & tested by the community » Fixed

committed.

gielfeldt’s picture

gielfeldt commented

FYI, I've ported the changes to 6.x and 8.x

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.