Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEPS045 update addons for Enhanced Places #214

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

GEPS045 update addons for Enhanced Places #214

wants to merge 4 commits into from

Conversation

prculley
Copy link
Contributor

@prculley prculley commented Jul 9, 2019

This PR is intended to go along with gramps-project/gramps#809 and MUST NOT be commited until that PR is accepted and commited.

I've provided it at the moment to allow testing of some of these addons with the main PR, in particular GetGOV and PlaceCleanup, which have been modified to support the primary PR and fully utilize the main PRs features.

@romjerome
Copy link
Contributor

romjerome commented Jul 13, 2019

MUST NOT be commited until that PR is accepted and commited

Sorry I did some commits on lxml gramplet related to GEPS 045 but they are minor and could be easily reverted. Only one real change related to master branch (an exception with empty source title).

Also have a strange issue around dummyDB on etree gramplet, getting an error by calling 'self.dbstate.db.get_number_of_citations()' which seems specific to master branch as this worked before (update : can also reproduce it on gramps50 branch; so these specific issues started some years ago.)

@prculley
Copy link
Contributor Author

@romjerome It appears you made your changes in master branch. Perhaps you can rebase your work on top of mine and push to this branch? Probably not too important, since I suspect you are the only one using these Gramplets...

Regarding the dummyDB; since this db is only active when the main dbs are closed, and dummyDB doesn't implement any of the 'get_number_of_???' methods, I'm surprised you only saw the issue with citations. I would have expected 'NotImplementedError' for all the calls. You should probably put all the print statements inside of a "if self.dbstate.db.db_is_open:" like you did for the tags.

@romjerome
Copy link
Contributor

romjerome commented Jul 14, 2019

I would have expected 'NotImplementedError' for all the calls.

Yes, it is logical to get the 'NotImplementedError' according to current code,
but that should not be specific to citation objects on this gramplet.

My first impression was that something has been improved around environment for gedcom support, which might (could?) explain that the method for citation has this custom behaviour?
e.g., most objects could use a new way and citation object has not been completely updated? I need to look at piece of code using this counting method. Also need to look at a proper use of 'db.surname_list' which is also broken on my experimental etree gramplet.

I suspect you are the only one using these Gramplets...

Yes, maybe !
They could be hidden or removed.

As they provided some informations without advanced coding or large investigations for people discovering gramps, maybe someone could be happy to look at them for its own use.

Thanks.

@romjerome
Copy link
Contributor

romjerome commented Jul 14, 2019

FutureWarning:
The behavior of this method will change in future versions.
Use specific 'len(elem)' or 'elem is not None' test instead.

oh, yes I also need to look at my sample (gramps xml file) as
this old comment has been added some years ago ...

Often using len() as the simplest method for counting
but also some ugly part of code for returing collected data
on this etree gramplet (lazy way). Maybe a re-factoring
could be easier than looking at some specific issues
with this current code!

@romjerome
Copy link
Contributor

romjerome commented Jul 14, 2019

Note, one (feature) on etree gramplet lets help me to check structure and content of my data after an upgrade of gramps version : counting references (links or xlinks) not primary object or tags, often secondary objects but also some hidden data like attributes or specific texts.

It was a quick and dirty check based on counters.

others = len(entries) - (len(tags) + len(events) + len(people) + len(families) + len(sources) + 
len(citations) + len(places) + len(objects) + len(repositories) + len(notes))

So, it is not a proper and advanced DB check, only an other test before a migration.
More basic than ImportMerge tool, only more accessible for checking my archives.

@romjerome
Copy link
Contributor

romjerome commented Jul 22, 2019

@prculley

Regarding the dummyDB; since this db is only active when the main dbs are closed, and dummyDB doesn't implement any of the 'get_number_of_???' methods, I'm surprised you only saw the issue with citations.

A pull request via github (online) has been generated.

index 89c93ab..3f39fbc 100644
--- a/gramps/gen/db/dummydb.py
+++ b/gramps/gen/db/dummydb.py
@@ -757,6 +757,14 @@ class DummyDb(M_A_M_B("NewBaseClass", (DbReadBase, Callback, object,), {})):
             LOG.warning("database is closed")
         return 0
 
+    def get_number_of_citations(self):
+        """
+        Return the number of citations currently in the database.
+        """
+        if not self.db_is_open:
+            LOG.warning("database is closed")
+        return 0
+
     def get_number_of_tags(self):
         """
         Return the number of tags currently in the database.
index 731fcd7..c6f3139 100644
--- a/gramps/plugins/db/bsddb/test/db_test.py
+++ b/gramps/plugins/db/bsddb/test/db_test.py
@@ -81,6 +81,7 @@ class DbTest(unittest.TestCase):
         "get_number_of_places",
         "get_number_of_repositories",
         "get_number_of_sources",
+        "get_number_of_citations",
         "get_number_of_tags",
         "get_media_from_gramps_id",
         "get_media_from_handle",

I can guess why this 'citation' method is not useful with gedcom (import, export) and only saw an additionnal use (on specific gramps/plugins/tool/mergecitations tool). So, currently it seems that the method is just missing on dummydb and that's why I got the cryptic issue. At least it should return 0 if there is no loaded db, like for others primary objects, tags and maybe surnames for the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants