jenkins-bot has submitted this change. ( 
https://gerrit.wikimedia.org/r/c/pywikibot/core/+/965243 )

Change subject: [IMPR] Improve handling of uninitialized MediaInfo
......................................................................

[IMPR] Improve handling of uninitialized MediaInfo

Make it possible to retrieve just the id for non-existing
mediainfo's of existing files, and use this as an internal
assertion. It will also throw an error for invalid
mediainfo references.

Drop MediaInfo.get_data_for_new_entity because it will
not be called anymore.

Change-Id: I7ad7396708e9fc7fcd98274c3668f07e25a1be30
---
M pywikibot/page/_wikibase.py
M tests/file_tests.py
2 files changed, 101 insertions(+), 38 deletions(-)

Approvals:
  Zache-tool: Looks good to me, but someone else must approve
  Xqt: Looks good to me, approved
  jenkins-bot: Verified




diff --git a/pywikibot/page/_wikibase.py b/pywikibot/page/_wikibase.py
index 753dc3f..bbfd71f 100644
--- a/pywikibot/page/_wikibase.py
+++ b/pywikibot/page/_wikibase.py
@@ -130,14 +130,17 @@
     def __getattr__(self, name):
         if name in self.DATA_ATTRIBUTES:
             if self.getID() == '-1':
-                for key, cls in self.DATA_ATTRIBUTES.items():
-                    setattr(self, key, cls.new_empty(self.repo))
+                self._initialize_empty()
                 return getattr(self, name)
             return self.get()[name]

         raise AttributeError("'{}' object has no attribute '{}'"
                              .format(self.__class__.__name__, name))

+    def _initialize_empty(self):
+        for key, cls in self.DATA_ATTRIBUTES.items():
+            setattr(self, key, cls.new_empty(self.repo))
+
     def _defined_by(self, singular: bool = False) -> dict:
         """
         Internal function to provide the API parameters to identify the entity.
@@ -355,7 +358,7 @@
         """
         Return the full concept URI.

-        :raise NoWikibaseEntityError: if this entity doesn't exist
+        :raise NoWikibaseEntityError: if this entity's id is not known
         """
         entity_id = self.getID()
         if entity_id == '-1':
@@ -370,6 +373,7 @@
     .. versionadded:: 6.5
     """

+    entity_type = 'mediainfo'
     title_pattern = r'M[1-9]\d*'
     DATA_ATTRIBUTES = {
         'labels': LanguageDict,
@@ -378,12 +382,39 @@

     def __getattr__(self, name):
         if name == 'claims':  # T149410
-            name = 'statements'
-            if hasattr(self, name):
-                return getattr(self, name)
+            return self.statements
+
+        if name in self.DATA_ATTRIBUTES:
+            if not self.exists():
+                self._assert_has_id()
+                self._initialize_empty()
+            return getattr(self, name)

         return super().__getattr__(name)

+    def _assert_has_id(self):
+        if self.id != '-1':
+            return
+
+        if not self.file.exists():
+            exc = NoPageError(self.file)
+            raise NoWikibaseEntityError(self) from exc
+
+        self.id = 'M' + str(self.file.pageid)
+
+    def _defined_by(self, singular: bool = False) -> dict:
+        """
+        Internal function to provide the API parameters to identify the entity.
+
+        :param singular: Whether the parameter names should use the singular
+                         form
+        :raise NoWikibaseEntityError: if this entity is associated with
+                                      a non-existing file
+        :return: API parameters
+        """
+        self._assert_has_id()
+        return super()._defined_by(singular)
+
     @property
     def file(self) -> FilePage:
         """Get the file associated with the mediainfo."""
@@ -397,7 +428,8 @@
                 pywikibot.error(msg)
                 raise Error(msg)

-            page_id = self.getID(numeric=True)
+            # avoid recursion with self.getID()
+            page_id = int(self.id[1:])
             result = list(self.repo.load_pages_from_pageids([page_id]))
             if not result:
                 raise Error(f'There is no existing page with id "{page_id}"')
@@ -410,27 +442,24 @@

         return self._file

-    def get_data_for_new_entity(self) -> dict:
-        """Return data required for creation of a new mediainfo."""
-        self.id = 'M' + str(self.file.pageid)
-        self._content = {}
-        return super().get()
-
     def get(self, force: bool = False) -> dict:
         """Fetch all MediaInfo entity data and cache it.

+        .. note:: This method may raise exception even if the associated file
+           exists because the mediainfo may not have been initialized yet.
+           :attr:`labels` and :attr:`statements` can still be accessed and
+           modified. :meth:`exists` suppresses the exception.
+
+        .. note:: dicts returned by this method are references to content
+           of this entity and their modifying may indirectly cause
+           unwanted change to the live content
+
         :param force: override caching
         :raise NoWikibaseEntityError: if this entity doesn't exist
         :return: actual data which entity holds
         """
         if self.id == '-1':
-            if force:
-                if not self.file.exists():
-                    exc = NoPageError(self.file)
-                    raise NoWikibaseEntityError(self) from exc
-                # get just the id for Wikibase API call
-                self.id = 'M' + str(self.file.pageid)
-            else:
+            if not force:
                 try:
                     data = self.file.latest_revision.slots['mediainfo']['*']
                 except NoPageError as exc:
@@ -443,6 +472,8 @@
                 self._content = jsonlib.loads(data)
                 self.id = self._content['id']

+            self._assert_has_id()
+
         return super().get(force=force)

     def getID(self, numeric: bool = False):
@@ -450,9 +481,10 @@
         Get the entity identifier.

         :param numeric: Strip the first letter and return an int
+        :raise NoWikibaseEntityError: if this entity is associated with
+                                      a non-existing file
         """
-        if self.id == '-1':
-            self.get()
+        self._assert_has_id()
         return super().getID(numeric=numeric)


diff --git a/tests/file_tests.py b/tests/file_tests.py
index 911c595..8a252c4 100755
--- a/tests/file_tests.py
+++ b/tests/file_tests.py
@@ -13,6 +13,7 @@

 import pywikibot
 from pywikibot.exceptions import (
+    Error,
     NoPageError,
     NoWikibaseEntityError,
     PageRelatedError,
@@ -351,7 +352,7 @@
         self.assertTrue(item.file is page)
         self.assertEqual('-1', item.id)
         item.get()
-        self.assertEqual('M14634781', item.getID())
+        self.assertEqual('M14634781', item.id)
         self.assertIsInstance(
             item.labels, pywikibot.page._collections.LanguageDict)
         self.assertIsInstance(
@@ -371,6 +372,15 @@
         del item._file
         self.assertEqual(page, item.file)

+    def test_data_item_not_file(self):
+        """Test data item with invalid pageid."""
+        item = pywikibot.MediaInfo(self.site, 'M1')  # Main Page
+        with self.assertRaises(Error):
+            item.file
+        with self.assertRaises(NoWikibaseEntityError):
+            item.get()
+        self.assertFalse(item.exists())
+
     def test_data_item_when_no_file_or_data_item(self):
         """Test data item associated to file that does not exist."""
         page = pywikibot.FilePage(self.site,
@@ -381,6 +391,8 @@

         with self.assertRaises(NoWikibaseEntityError):
             item.get()
+        with self.assertRaises(NoWikibaseEntityError):
+            item.labels

     def test_data_item_when_file_exist_but_without_item(self):
         """Test if data item is missing from file."""
@@ -394,24 +406,26 @@

         # Seek to first page without mediainfo.
         for page in gen:
-            if 'mediainfo' not in page.latest_revision.slots:
-                item = page.data_item()
-                self.assertIsInstance(item, pywikibot.MediaInfo)
+            if 'mediainfo' in page.latest_revision.slots:
+                continue

-                # Get fails as there is no mediainfo.
-                with self.assertRaises(NoWikibaseEntityError):
-                    item.get()
+            item = page.data_item()
+            self.assertIsInstance(item, pywikibot.MediaInfo)

-                # Create new empty mediainfo.
-                item.get_data_for_new_entity()
-                self.assertIsInstance(
-                    item.labels, pywikibot.page._collections.LanguageDict)
-                self.assertIsInstance(
-                    item.statements,
-                    pywikibot.page._collections.ClaimCollection)
+            # Get fails as there is no mediainfo.
+            with self.assertRaises(NoWikibaseEntityError):
+                item.get()

-                # break the loop after checking first file
-                break
+            self.assertFalse(item.exists())
+            self.assertEqual(f'M{page.pageid}', item.id)
+            self.assertIsInstance(
+                item.labels, pywikibot.page._collections.LanguageDict)
+            self.assertIsInstance(
+                item.statements,
+                pywikibot.page._collections.ClaimCollection)
+
+            # break the loop after checking first file
+            break

     def test_data_list_to_dict_workaround(self):
         """Test that T222159 workaround converts [] to {}."""

--
To view, visit https://gerrit.wikimedia.org/r/c/pywikibot/core/+/965243
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.wikimedia.org/r/settings

Gerrit-Project: pywikibot/core
Gerrit-Branch: master
Gerrit-Change-Id: I7ad7396708e9fc7fcd98274c3668f07e25a1be30
Gerrit-Change-Number: 965243
Gerrit-PatchSet: 7
Gerrit-Owner: Matěj Suchánek <[email protected]>
Gerrit-Reviewer: Xqt <[email protected]>
Gerrit-Reviewer: Zache-tool <[email protected]>
Gerrit-Reviewer: jenkins-bot
Gerrit-MessageType: merged
_______________________________________________
Pywikibot-commits mailing list -- [email protected]
To unsubscribe send an email to [email protected]

Reply via email to