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

BibTeX comma first syntax #49

Merged
merged 6 commits into from
Dec 22, 2014
Merged

Conversation

grochmal
Copy link
Contributor

Comma first syntax is allowed by Patashnik's original BibTeX implementation, and it is popular among people that use functional languages. Notably the Glasgow Haskell Compiler at the University of Glasgow. Support for such input was needed by me for a django project that is using the bibtex parser. The changes in this pull request are the hacks I did to make bibtexparser work in an environment where the comma first syntax is often used.

To ensure that the changes do not break the package in any way I've run all unittests under both python 2.7 and python 3.4 . Also, to ensure the proper functioning of the comma fisrt feature in the future I've added two more unittests (one for the bparser and one for the bwriter) focused on comma first syntax.

I am not sure of how you perform the unitesting of the bibtexparser package (as I have never used tox), yet the following simple script (when copied to the root of the project) does the job of running all tests:

#!/bin/sh
DIR=$(dirname $(readlink -e "$0"))
cd $DIR
touch bibtexparser/tests/__init__.py
P2=$(which python2 2>/dev/null)
P3=$(which python3 2>/dev/null)
for p in $P2 $P3; do
  echo $p -m unittest discover -t . -s bibtexparser/tests -p 'test_*.py'
  $p -m unittest discover -t . -s bibtexparser/tests -p 'test_*.py'
done
rm bibtexparser/tests/__init__.py

This script was not added to the pull request, as it is not part of the given feature. On the other hand I'd suggest adding this script, or better, some instructions on using tox to the README of the project.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling b0e6745 on grochmal:comma_first into 4c51cf7 on sciunto-org:master.

@@ -47,6 +47,10 @@ def __init__(self):
self.entry_separator = '\n'
#: Tuple of fields for ordering entries. Set to `None` to disable sorting. Default: BibTeX key `('id', )`.
self.order_entries_by = ('id', )
#: BibTeX syntax allows comma first syntax
#: (common in functional languages), use this to enble
Copy link
Member

Choose a reason for hiding this comment

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

Typo here: enable

@sciunto
Copy link
Member

sciunto commented Nov 28, 2014

Looks great! Thank you. I just added few minor comments.

About the tests, each time you submit a pull request, travis is triggered and you can see the result of the tests. I'll add some links to relevant documentations (#50).

Also, would you like to add a new paragraph explaining your feature in the documentation?
You can also add your name/nickname to CONTRIBUTORS.txt :)

@grochmal
Copy link
Contributor Author

grochmal commented Dec 1, 2014

Cool, and thanks for the comments. Your comments are minor, so I already fixed most of them.

On the documentation, I hoped that the #: comments will be caught by sphinx and that will be enough. This is because on the parser side, not supporting a certain syntax (a rather arcane syntax, but still a used one) of BibTeX was a small bug (I believe). On the writer side I added the feature of writing in this comma first syntax but the writer has not a section in the tutorial.

Thinking about it a little, what I will do is to add a section entitled Using the writer to the tutorial. This section will describe the basic usage of the writer and document the comma first syntax in a sentence or two.

@sciunto
Copy link
Member

sciunto commented Dec 3, 2014

You learnt me something about the #: syntax.

That's true, there is not writer entry in the documentation, and it's missing :)
If you would like to fill the gap, that would be awesome :)

Thanks!

@sciunto
Copy link
Member

sciunto commented Dec 13, 2014

Just to follow up on this. I guess you made some modifications without pushing them...

Thanks. :)

@grochmal
Copy link
Contributor Author

I got focused on some report I need to deliver on the 18th (read: chased by people that I should do a report), so I left the code half way.

My apologies for that. I promise it'll be pulled next weekend (20th-21st).

PS: Am aware of possible problems of integrating several pieces of code if several pulls happen on the master branch without pushing it first. And I see two pull requests added recently. If you want to pull them first I can deal with geting the new code into my workarea and re-check it all before updating this pull request.

@sciunto
Copy link
Member

sciunto commented Dec 16, 2014

I merged one of the PR. The other one involves only one line. Should be ok ;)

Thanks and no pb for the delay. Take the time you need.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9c473bf on grochmal:comma_first into * on sciunto-org:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 18dafd1 on grochmal:comma_first into * on sciunto-org:master*.

@grochmal
Copy link
Contributor Author

Sorry again for the huge delay. The merge was not trivial but it wasn't anything that git or unittest would not discover. On the bright side, the tougher merge made me learn more about bibtexparser's codebase.

I have added a new section to the tutorial.rst and needed to update bibtexparser.rst to add a label referenced from this new section. The documentation generation was tested under the html and latexpdf make targets, so it should be alright.

I also took the liberty to update a comment in bibdatabase.py which was not reflecting the new ID and ENTRYTYPE dictionary keys. I believe that the file customization.py at line 116 has a similar problem, i.e. the old type is being used instead of ENTRYTYPE. But am not really sure, and that file is not part of any important module, so I left it. Please have a quick look at it.

If you find some bad wording in the new documentation section, feel free to chase me. They're totally my fault.

Have a Merry Christmas if we do not cross post before that.

@sciunto
Copy link
Member

sciunto commented Dec 22, 2014

Many many thanks! Looks great.

I merge this one and fix customization.py.

Merry Christmas.

sciunto added a commit that referenced this pull request Dec 22, 2014
@sciunto sciunto merged commit 723e220 into sciunto-org:master Dec 22, 2014
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.

3 participants