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

bugfix for merge-other #45

Merged
merged 4 commits into from
Aug 19, 2024
Merged

bugfix for merge-other #45

merged 4 commits into from
Aug 19, 2024

Conversation

syrte
Copy link
Contributor

@syrte syrte commented Aug 16, 2024

Solved #44
Editing the property bib.entries_dict doesn't work. Use the underlying bib._entries_dict instead!

This wired bug is found by inserting the debugging code:

            print("DEBUG:", key in bib.entries_dict, key in [_['ID'] for _ in bib.entries])
            print("{}: FOUND IN OTHER BIB SOURCE, MERGED".format(key))

I realized that editing the property bib.entries_dict will not change bib._entries_dict.

syrte added 2 commits August 16, 2024 07:35
Editing the property `bib.entries_dict` doesn't work. Use the underlying `bib._entries_dict` instead!
I found that when the only changes are merging items from another bib, adstex does not write the file unless I delete the output file...
This is a rare case maybe not worth a fix.
Anyway, it would be good at least to remind users nothing to be written.
@syrte
Copy link
Contributor Author

syrte commented Aug 16, 2024

Why editing the property bib.entries_dict does not work anymore:

In bibtexparser.bibdatabase (bibtexparser.__version__ = '1.4.1'),

class BibDatabase(object):
    """
    Bibliographic database object that follows the data structure of a BibTeX file.
    """

    def __init__(self):
        #: List of BibTeX entries, for example `@book{...}`, `@article{...}`, etc. Each entry is a simple dict with
        #: BibTeX field-value pairs, for example `'author': 'Bird, R.B. and Armstrong, R.C. and Hassager, O.'` Each
        #: entry will always have the following dict keys (in addition to other BibTeX fields):
        #:
        #: * `ID` (BibTeX key)
        #: * `ENTRYTYPE` (entry type in lowercase, e.g. `book`, `article` etc.)
        self.entries = []
        self._entries_dict = {}
...
    def get_entry_dict(self):
        """Return a dictionary of BibTeX entries, where dict key is the BibTeX entry key.

        This method re-creates the dict every time it is called,
        hence subsequent calls should be avoided with large databases.
        """
        self._entries_dict = dict()
        for entry in self.entries:
            self._entries_dict[entry['ID']] = entry
        return self._entries_dict

    entries_dict = property(get_entry_dict)

Every call to BibDatabase.entries_dict is fresh. So any changes to entries_dict do not change the internal status of the database object.
Therefore, we should use BibDatabase.entries directly. entries_dict is only provided as a (read only) interface for convenience. ._entries_dict is slightly more persistent, but also will be refreshed by calling entries_dict.

Copy link
Owner

@yymao yymao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this issue. Given the behavior of entries_dict, I wonder if we can just modify entries in this case. See my suggested edit (I have not tested it).

adstex.py Outdated
Comment on lines 457 to 458
bib._entries_dict[key] = bib_other.entries_dict[key]
bib.entries = list(bib._entries_dict.values())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bib._entries_dict[key] = bib_other.entries_dict[key]
bib.entries = list(bib._entries_dict.values())
bib.entries.append(bib_other.entries_dict[key])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense to me.
It works in my examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realized that I could have simply accepted your suggested edit
first time met this function :)

Copy link
Owner

@yymao yymao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing!

@yymao yymao merged commit 5a001a8 into yymao:master Aug 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants