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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heykarthikwithu created an issue. See original summary.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
CreditAttribution: heykarthikwithu as a volunteer and at Valuebound commented
Assigned: heykarthikwithu » Unassigned
Status: Active » Needs review
FileSize
2663256-2.patch5.72 KB
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
FileSize
2663256-3.patch8.15 KB
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 🇮🇹
CreditAttribution: mondrake commented
FileSize
interdiff_2-3.txt3.34 KB

Wrong interdiff, sorry

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
CreditAttribution: heykarthikwithu as a volunteer and at Valuebound 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 🇮🇹
CreditAttribution: mondrake 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 🇮🇹
CreditAttribution: mondrake 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 🇮🇹
CreditAttribution: mondrake commented
FileSize
2663256-10-test-only.patch2.47 KB
2663256-3-test-only.patch10.62 KB
2663256-10.patch10.51 KB
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 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake 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 🇮🇹
CreditAttribution: mondrake commented
Issue tags: -Needs tests
slashrsm’s picture

slashrsm CreditAttribution: slashrsm as a volunteer 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 🇮🇹
CreditAttribution: mondrake 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 CreditAttribution: slashrsm as a volunteer commented

It must be a drupal.org bug or something.

Status: Fixed » Closed (fixed)

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