Repository: scrapy/scrapy
Pull request #5963 (2023-06-27 19:19:12)

fix: Return value instead of `SettingsAttribute` object when using `pop` method
As reported on https://github.com/scrapy/scrapy/issues/5959#issuecomment-1609832116, the implementation of `pop` method is returning `SettingsAttribute` object instead of the value itself which break interface. Mea culpa, sorry! 😅 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/scrapy/scrapy/pull/5963?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) Report
> Merging [#5963](https://app.codecov.io/gh/scrapy/scrapy/pull/5963?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) (a252597) into [master](https://app.codecov.io/gh/scrapy/scrapy/commit/510574216d70ec84d75639ebcda360834a992e47?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) (5105742) will **decrease** coverage by `2.02%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head a252597 differs from pull request most recent head 90cfbd1. Consider uploading reports for the commit 90cfbd1 to get more accurate results

```diff
@@            Coverage Diff             @@
##           master    #5963      +/-   ##
==========================================
- Coverage   89.19%   87.17%   -2.02%     
==========================================
  Files         162      162              
  Lines       11287    11287              
  Branches     1833     1833              
==========================================
- Hits        10067     9840     -227     
- Misses        928     1156     +228     
+ Partials      292      291       -1     
```

| [Impacted Files](https://app.codecov.io/gh/scrapy/scrapy/pull/5963?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy) | Coverage Δ | |
|---|---|---|
| [scrapy/settings/\_\_init\_\_.py](https://app.codecov.io/gh/scrapy/scrapy/pull/5963?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy#diff-c2NyYXB5L3NldHRpbmdzL19faW5pdF9fLnB5) | `98.34% <100.00%> (ø)` | |

... and [11 files with indirect coverage changes](https://app.codecov.io/gh/scrapy/scrapy/pull/5963/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy)


Patch:
diff --git a/scrapy/settings/__init__.py b/scrapy/settings/__init__.py
index 8b3bdbabe..cc44d67e8 100644
--- a/scrapy/settings/__init__.py
+++ b/scrapy/settings/__init__.py
@@ -449,12 +449,12 @@ class BaseSettings(MutableMapping):
 
     def pop(self, name, default=__default):
         try:
-            value = self.attributes[name]
+            value = self.attributes[name].value
         except KeyError:
             if default is self.__default:
                 raise
 
-            return SettingsAttribute(default, get_settings_priority("project"))
+            return default
         else:
             self.__delitem__(name)
             return value


Test patch:
diff --git a/tests/test_settings/__init__.py b/tests/test_settings/__init__.py
index bb6dc67fa..5fc825393 100644
--- a/tests/test_settings/__init__.py
+++ b/tests/test_settings/__init__.py
@@ -457,19 +457,15 @@ class SettingsTest(unittest.TestCase):
         with self.assertRaises(KeyError):
             settings.pop("DUMMY_CONFIG")
 
-        dummy_config = settings.pop("DUMMY_CONFIG", "dummy_value")
-
-        self.assertEqual(
-            repr(dummy_config), "<SettingsAttribute value='dummy_value' priority=20>"
-        )
-        self.assertEqual(dummy_config.value, "dummy_value")
+        dummy_config_value = settings.pop("DUMMY_CONFIG", "dummy_value")
+        self.assertEqual(dummy_config_value, "dummy_value")
 
     def test_pop_item_with_immutable_settings(self):
         settings = Settings(
             {"DUMMY_CONFIG": "dummy_value", "OTHER_DUMMY_CONFIG": "other_dummy_value"}
         )
 
-        self.assertEqual(settings.pop("DUMMY_CONFIG").value, "dummy_value")
+        self.assertEqual(settings.pop("DUMMY_CONFIG"), "dummy_value")
 
         settings.freeze()