Skip to content
New issue

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

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

Already on GitHub? # to your account

Custom writer example in docs #337

Open
mscheltienne opened this issue Oct 19, 2022 · 3 comments
Open

Custom writer example in docs #337

mscheltienne opened this issue Oct 19, 2022 · 3 comments

Comments

@mscheltienne
Copy link

Currently, defining a custom writer is possible but not straightforward.

  1. It is not clear that you have to initialize a writer and then change the attributes that were initialized. Having the attributes exposed as arguments of __init__(), with detailed docstrings used by the sphinx-build, might improve the experience. Example using numpy style docstrings:
class BibTexWriter(object):
    """
    Writer to convert a :class:`BibDatabase` object to a string or file formatted as a BibTeX file.

    Parameters
    -----------
    param1 : type
        description
    param2 : type
        description
    ...
    """

    def __init__(self, param1, param2, ...):
        pass
  1. Sorting of the fields is hidden in the undocumented SortingStrategy class.

  2. Most people will not define a custom writer and are simply going to use the dump function. The most common configuration should be accessible directly at the dump function level, e.g. sorting of entries and sorting of fields. Example:

def dump(
    bib_database: BibDatabase, 
    bibtex_file: TextIO, 
    writer: Optional[BibTexWriter] = None,
    order_entries: Optional[Tuple[str]] = None,
    order_fields: Optional[str] = None,
):
    pass

In the example above, None corresponds to "preserve" order. order_entries corresponds to the writer's attribute order_entries_by and order_fields corresponds to the writer's attribute display_order_sorting.

Originally posted in #336
x-ref with v2: #318

@MiWeiss
Copy link
Collaborator

MiWeiss commented Oct 19, 2022

Thanks for opening the issue. Let me try to think this through 😄

Some concerns:

  1. The dump method must be backwards compatible. Hence, the default cannot be "preserve", but must equal the current writer default (I also think that preserve would be the better default, but backwards compatibility is of higher importance here).
  2. Given that we have exposed the SortingStrategy enum, the API should support it. How about using both, i.e., Union[SortingStrategy, str]?
  3. It may be a bit confusing to the user that we're exposing some writer customization through dump, and others not.
  4. What are we doing if a custom writer is provided, and also order_entries and order_fields?

While I think we'll get around my concerns 1 and 2. The remaining two are more tricky. Hence an alternative propositions: As I understand it, your main reason for proposing this is the fact that it is not obvious that/how a writer should be customized when using the dump function. I agree - that must change. How about - instead of code changes - we explain this in the dump docstring, including an example for the most common requirements such as order_entries and order_fields?

Either way (code or docs change): Would you be willing to provide a PR for this?

Note. This is not directly relevant to #318 which is a full 100% rewrite and where the dump method will have a different signature anyways. But of course, it's good to keep it in the back of our minds.

@MiWeiss MiWeiss changed the title Simplify the writer public API New arguments on bibtexparser.dump Oct 19, 2022
@mscheltienne
Copy link
Author

mscheltienne commented Oct 19, 2022

The dump method must be backwards compatible. Hence, the default cannot be "preserve", but must equal the current writer default (I also think that preserve would be the better default, but backwards compatibility is of higher importance here).

Simple enough, order_entries: Optional[Tuple[str]] = ("ID",) and order_fields: Optional[str] = "alphabetical_asc".

Given that we have exposed the SortingStrategy enum, the API should support it. How about using both, i.e., Union[SortingStrategy, str]?

It's exposed.. but it's not part of the documentation. So you need to either know it exists or dig in the files to find it. IMO, that's why a str makes more sense.

It may be a bit confusing to the user that we're exposing some writer customization through dump, and others not.

IMO, this one is easily solved by adding in the docstring a note about the behavior. Something like:

Notes
------
Only customization of the sorting order for entries and fields is available at the function level, with 
the arguments ``order_entries`` and ``order_fields``. To customize further the writer's behavior, 
a :class:`BibTexWriter` must be defined and provided in the argument ``writer``.

.. code-block:: python
   
    # example

What are we doing if a custom writer is provided, and also order_entries and order_fields?

I would raise an error message inviting the user to provide one or the other. Some people prefer to silently merge, or chose one input over the other; personally, I don't like to solve conflicts silently and I prefer to raise.


All that said, if there is a complete refactor version coming up, I don't see the added value in continuing the development of version 1. I would consider the points above as 'to keep in mind' for the refactor version.

@MiWeiss
Copy link
Collaborator

MiWeiss commented Oct 19, 2022

Sounds fair - that would have been a way to go, but I also agree that investing too much time for a v1 change which will only be used by a handful of people is probably not worth it. 👍

All that said, if there is a complete refactor version coming up, I don't see the added value in continuing the development of version 1.

Well, that's a long process, so if we could do the minimal thing and improve the dump docstring that would be a quick win. I'm this renaming this issue accordingly and opening it up for volunteers to extend the dump docstring with more information and an example of how to perform sorting using a custom writer instance.

@mscheltienne thanks again for bringing this up

@MiWeiss MiWeiss changed the title New arguments on bibtexparser.dump Extend documentation of bibtexparser.dump: Explain custom writer with example Oct 19, 2022
@MiWeiss MiWeiss changed the title Extend documentation of bibtexparser.dump: Explain custom writer with example [v1] Custom writer example in docs May 30, 2023
@MiWeiss MiWeiss added v1 and removed hacktoberfest labels May 30, 2023
@MiWeiss MiWeiss changed the title [v1] Custom writer example in docs Custom writer example in docs May 30, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants