Skip to content

✨ Add option to adjust alignment of text of multi-line values.#290

Merged
MiWeiss merged 4 commits intosciunto-org:masterfrom
michaelfruth:master
Aug 22, 2022
Merged

✨ Add option to adjust alignment of text of multi-line values.#290
MiWeiss merged 4 commits intosciunto-org:masterfrom
michaelfruth:master

Conversation

@michaelfruth
Copy link
Copy Markdown
Collaborator

A new option align_multiline_values is added to BibTexWriter to enable whether text of multiline values should be aligned on top of each other. Indentation also considered _max_field_width.

Current behaviour:
BibTeX entry:

@article{test,
  title     = {Hello World. I'm a very very very
  very very very very long title. Hello Bibtex Parser!
  This is still a long title!},
}

Parsing with BibtexParser will result in:

@article{test,
 title = {Hello World. I'm a very very very
very very very very long title. Hello Bibtex Parser!
This is still a long title!}
}

When align_multiline_values is set to true, the output will be the
following:

@article{test,
 title = {Hello World. I'm a very very very
          very very very very long title. Hello Bibtex Parser!
          This is still a long title!}
}

@MiWeiss
Copy link
Copy Markdown
Collaborator

MiWeiss commented Jul 9, 2022

Hi @michaelfruth

I just joined the project - sorry for the long wait.

Before I start the full review - would you mind adding some unit tests to cover this functionality? Please also make sure the already existing indentation functionality works well with this.

Also, please pull master to enable running the test suite on the PR :-)

Thanks for opening the PR!

@michaelfruth
Copy link
Copy Markdown
Collaborator Author

Thanks! I'll probably add some unit tests next week or two.

@MiWeiss
Copy link
Copy Markdown
Collaborator

MiWeiss commented Aug 4, 2022

Hi @michaelfruth
Do you have any updates?

@michaelfruth
Copy link
Copy Markdown
Collaborator Author

I'm lacking behind my schedule. I didn't forget to write some tests, try to do this until the end of this week.

@MiWeiss
Copy link
Copy Markdown
Collaborator

MiWeiss commented Aug 9, 2022

No worries! Thanks for the heads up, and for your contribution.

A new option align_multiline_values is added to enable whether text of
multiline values should be aligned on top of each other.

Current behaviour:
BibTeX entry:
@Article{test,
  title     = {Hello World. I'm a very very very
  very very very very long title. Hello Bibtex Parser!
  This is still a long title!},
}

Parsing with BibtexParser will result in:
@Article{test,
 title = {Hello World. I'm a very very very
very very very very long title. Hello Bibtex Parser!
This is still a long title!}
}

When align_multiline_values is set to true, the output will be the
following:
@Article{test,
 title = {Hello World. I'm a very very very
          very very very very long title. Hello Bibtex Parser!
          This is still a long title!}
}
@michaelfruth
Copy link
Copy Markdown
Collaborator Author

I've added some tests to test:

  1. the basic functionality
  2. the new functionality with align_values and indent

I have also encountered this behavior #315 , I'm not quite sure if this is expected.

Copy link
Copy Markdown
Collaborator

@MiWeiss MiWeiss left a comment

Choose a reason for hiding this comment

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

lgtm. found just one tiny issue in an inline comment.

If you agree, feel free to accept my suggestion. This will then be merged.

Comment thread bibtexparser/bwriter.py Outdated
@MiWeiss MiWeiss changed the title Add option to adjust alignment of text of multi-line values. ✨ Add option to adjust alignment of text of multi-line values. Aug 18, 2022
Review comments

Co-authored-by: Michael Weiss <code@mweiss.ch>
Copy link
Copy Markdown
Collaborator

@MiWeiss MiWeiss left a comment

Choose a reason for hiding this comment

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

lgtm. thanks for your contribution!

@MiWeiss MiWeiss merged commit 3ef7b4d into sciunto-org:master Aug 22, 2022
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.

2 participants