Conversation
|
Travis not running tests? Shall we move it to Github Actions? |
|
I was thinking to only change the lexer pattern for the parser to accept braces; we shouldn't strip them off the key, because it would be unexpected for the parser to alter the contents (this would have to go into some kind of optional filter, e.g., Yes, I think we're still configured for travis-ci.org which has been suspended and moving to Github Actions would be desirable. I'll open an issue for it. |
|
ok! I'll help with CI first |
5a3c5f1 to
074da38
Compare
|
@inukshuk you can see the failing tests. Any idea of why including the braces in the regex pattern is breaking those tests? |
|
Hm, for some reason adding the braces seems to make the key pattern match even beyond the final comma -- it's not obvious on a quick glance why that would happen. I'll have to take a closer look. |
|
@inukshuk worth pursuing this issue? |
|
Sure. Maybe it's just that the pattern is thrown off because the |
- do not remove braces - fixes test helper
074da38 to
caf2253
Compare
|
I tried that but still one test that fails |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for allowing braces in BibTeX citation keys, which expands the valid character set for cite-key parsing.
- Updates lexer regex patterns to include curly braces as valid characters in citation keys
- Refactors key parsing logic into a dedicated method for better code organization
- Adds comprehensive test coverage for the new brace support functionality
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/bibtex/lexer.rb | Updates regex patterns to allow braces in keys and refactors key parsing into separate method |
| test/bibtex/test_lexer.rb | Adds test for brace support and updates test syntax to modern Minitest format |
| test/test_bibtex.rb | Enhances existing tests with better error message validation and updated test data |
| Gemfile | Adds logger dependency |
Comments suppressed due to low confidence (1)
test/test_bibtex.rb:119
*
@article{foo, },
| gemspec | ||
|
|
||
| gem 'json', '~>2.0', platforms: %i[mri_18 jruby] | ||
| gem 'logger' |
There was a problem hiding this comment.
Added to fix this warning:
warning: logger was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
| Timeout.timeout(2) do | ||
| BibTeX.parse(<<EOF, allow_missing_keys: true) | ||
| @article{}, | ||
| @article{foo, }, |
There was a problem hiding this comment.
This is the test that was failing, I couldn't find a proper way of fixing. Using an alternative test case here works.
|
@inukshuk can you check again when having some time? I gave it another try |
|
You can't allow braces with optional keys, otherwise for an entry like: @article{}It will parse the closing brace as the key. I added the braces to the default pattern for mandatory keys, but I remain doubtful that it's a good practice to use braces in keys. I'm not sure if it's valid or if there is useful reason to define bibtex entries with only a key and nothing else, because that would also not work for exampe: @article{myid}Because it would also add the closing brace to the key. So, personally, I'd be in favor of leaving the default pattern as is. Overwrite the pattern locally to support braces. |
|
Thanks @inukshuk! |
Closes #150