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

🐛 align_values allows int or bool, fixes previous min alignment #316

Merged
merged 2 commits into from
Sep 27, 2022

Conversation

michaelfruth
Copy link
Collaborator

Fixes #315

Aligns all values according to a given length by padding with single spaces. If align_values is true, the maximum number of characters used in any field name is used as the length. If align_values is a number, the greater of the specified value or the maximum number of characters used in any field name is used as the length.

NOTE:: This commit includes breaking changes: The ENTRYTYPE entry was considered for calculating the maximum number of characters for entries, which always lead to a minimum value of 9. Now, keys that are not written as BibTeX output are ignored, which leads to an exact computation of the number of characters for entries.

@MiWeiss MiWeiss self-requested a review September 6, 2022 06:46
@MiWeiss MiWeiss changed the title align_values allows int or bool 🐛 align_values allows int or bool, fixes previous min alignment Sep 7, 2022
@MiWeiss
Copy link
Collaborator

MiWeiss commented Sep 9, 2022

Hi @michaelfruth

I have not forgotten this one - and I have also noticed your comments in the other PRs :-)

Regarding this PR: There's an edge case I still need to test & lay out to you.
Regarding the other PRs: There's some longer thoughts I have organize in my head and then write down... :-)

I'm quite busy atm, but hope that I can get back with you ASAP. Sorry for the wait - and thanks for your contribution!

@MiWeiss
Copy link
Collaborator

MiWeiss commented Sep 14, 2022

Hi @michaelfruth

Excuse my late answer and thanks again for your PR.

There's an edge case of which I am not sure how it should be handled: Assume someone specifies an align_value=x with int x, but there is some key len(key) > x. As I understand it, the current implementation uses len(x), ignoring the user-passed value. As an alternative implementation, I could see that x is applied where possible, and only the too long key "exceeds it", e.g.

@entry{someentry,
lor    = "lorem ipsum",
lo     = "lorem ipsum",
lore   = "lorem ipsum",
loremipsum = "lorem ipsum",
l      = "lorem ipsum",
}

I guess this would be more closely to what I'd expect given the naming align_value. Also, it would probably be more useful for large to huge bibtex files, where a single key (like some unexpected value) should not mess up the formatting of the entire output.

What do you think? Any reasons to prefer the current implementation?

@MiWeiss
Copy link
Collaborator

MiWeiss commented Sep 22, 2022

@michaelfruth - I guess we'll be able to take this over to v2, hence I recommend we continue with this PR (also merging it into 1.x)

@MiWeiss
Copy link
Collaborator

MiWeiss commented Sep 25, 2022

Hi @michaelfruth

Not sure if this (again) ready for review. But in case it is - could you fix the merge conflict first? Would allow to run the actions :-)

Fixes sciunto-org#315

The `align_values` of the BibTexWriter now accepts a bool or int value.
If the bool value `true` is specified, the maximal number of characters used in any fieldname is used as length. If a integer value is specified, the greater of the specified
integer value and the overall maximal number of characters used in any
fieldname is used.

This commit also fixes the previous behaviour of align_values which
results in a breaking change. The
`ENTRYTYPE` entry was considered for calculating the maximal number of
characters, which always lead to a minimum value of `9`. Now, keys that
are not written into the BibTex output are ignored which leads to an
exact computation of the fieldname lengths.

Example:
```
@book{abc123,
 a         = {test},
 bb        = {longvalue}
}
@book{abc123,
 a  = {test},
 bb = {longvalue}
}
```
@michaelfruth
Copy link
Collaborator Author

Now PR is ready for review again 👍

@MiWeiss MiWeiss merged commit 2ba4323 into sciunto-org:master Sep 27, 2022
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.

align_values has minimum _max_field_width due to ENTRYPOINT pseudo field.
2 participants