Remove unused imports in the code base

CommentFileSizeAuthor
#10 interdiff_3-10.txt2.06 KBmondrake
#10 2663256-10.patch10.51 KBmondrake
#10 2663256-3-test-only.patch10.62 KBmondrake
#10 2663256-10-test-only.patch2.47 KBmondrake
#4 interdiff_2-3.txt3.34 KBmondrake
#3 interdiff_2-3.txt8.15 KBmondrake
#3 2663256-3.patch8.15 KBmondrake
#2 2663256-2.patch5.72 KBheykarthikwithu

Comments

heykarthikwithu created an issue. See original summary.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
Assigned: heykarthikwithu » Unassigned
Status: Active » Needs review
StatusFileSize
new 2663256-2.patch5.72 KB
mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 2663256-3.patch8.15 KB
new interdiff_2-3.txt8.15 KB

Thank you @heykarthikwithu

Actually I think we can do a little bit more, see patch attached. If this passes I will commit it.

This module is pretty new and we will have need for such cleanups in the future too as new image effects get added.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new interdiff_2-3.txt3.34 KB

Wrong interdiff, sorry

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented

This module is pretty new and we will have need for such cleanups in the future too as new image effects get added.

yes sure :)

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Fixed

Committed, thank you!

#6 ;)

  • mondrake committed 52aa1b8 on 
    Revert "Issue #2663256 by heykarthikwithu, mondrake: Remove unused...
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Fixed » Needs work
Issue tags: +Needs tests

Sorry, this broke the settings form, reverted. My fault, I removed the config factory injection which is needed by the parent constructor. This however tells us we are completely missing tests for the SettingsForm, which should then be added here.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 2663256-10-test-only.patch2.47 KB
new 2663256-3-test-only.patch10.62 KB
new 2663256-10.patch10.51 KB
new interdiff_3-10.txt2.06 KB

OK, fixed.

2663256-10-test-only.patch => is a test only patch (should be green) showing how SettingsForm in current HEAD is ok
2663256-3-test-only.patch => shows how the patch in #3 leads to failure of the SettingsForm (should be red)
2663256-10.patch => is the patch for review/commit (hopefully green ;))

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Remove unused imports in the code base » Remove unused imports in the code base, add tests for SettingsForm

The last submitted patch, 10: 2663256-3-test-only.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: -Needs tests
slashrsm’s picture

slashrsm commented
Status: Needs review » Fixed

Committed. Thank you!

  • slashrsm committed 66609ad on authored by mondrake 
    Issue #2663256 by mondrake, heykarthikwithu: Remove unused imports in...
mondrake’s picture

mondrake
Location 🇮🇹
commented

No idea why, but the date of the automatic commit message is set to Feb 3 where the commit was actually on Feb 9 :( ...

slashrsm’s picture

slashrsm commented

It must be a drupal.org bug or something.

Status: Fixed » Closed (fixed)

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