Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Requirement cleanup + Store everything as string #65

Closed
wants to merge 12 commits into from

2 participants

Daniel Mendler Sylvester Keil
Daniel Mendler

Hi,

I made multiple changes:

  • I cleaned up a bit the dependencies. Now missing deps don't lead to a failure. I also cleaned up the Rakefile and the tests slightly, reducing the number of hard deps.
  • I included the parser.rb in the repository. I prefer to include generated code in the repository to be independent of the racc version. I think most projects do it similarly. You also don't need racc to start developing then.
  • I made modifications to store everything only as string. This change is backward incompatible and a matter of taste and style. You could pull it when you release the gem with the name bibtex or as version 3.0, then the incompatibility wouldn't matter that much. I tend to use symbols only for few things where the set is limited. I think this style is also more common in most other libraries (e.g. XML/DOM libraries etc)

Looking forward to your response :)

Daniel

minad added some commits
Daniel Mendler minad minor Rakefile cleanup
- don't require yard
- don't require bundler
eb3c541
Daniel Mendler minad don't require minitest-colorize for tests
- also removed trailing whitespace
97b70af
Daniel Mendler minad lazy loading of multi json 2c33288
Daniel Mendler minad add the generated parsers to the repository
- This needs discussion!
- I think it is usually done like this in other projects
- Since the gem also includes the generated file, it is better like this. And then one
  also sees if something really changes in the parser.
- If someone starts developing, he doesn't need racc at first
7de60a9
Daniel Mendler minad remove latex-decode and multi_json as hard dependencies 70443f4
Daniel Mendler minad don't fail if filter cannot be loaded 5334c8d
Daniel Mendler minad don't store field keys as symbols 7a54df3
Daniel Mendler minad use BibTeX::Symbol instead of Symbol in values 37fe6c6
Daniel Mendler minad store type as string 674d853
Daniel Mendler minad use alias_method instead of alias
See http://stackoverflow.com/questions/4763121/should-i-use-alias-or-alias-method

I also find the use of the aliases inflationary. A lot of them could be removed imho.
6217d23
Daniel Mendler minad remove bibtex/ruby.rb file, assuming bibtex-ruby will be renamed 138ce92
Daniel Mendler minad don't require open-uri, the user can do that if he wants it 98fddad
Sylvester Keil
Owner

Awesome! I'll review the changes as soon as possible.

I'm fine with making the dependencies optional – we had a few users who urged against it, but you're right that for cases where you really don't need the LaTeX conversion it's not very co-operative to force the dependency.

Good call on including the generated parsers; that's something I have been meaning to do anyway to make it easier to run the tests on JRuby.

I'm a little bit concerned about the symbol/string changes, obviously; I hope we'll be able to keep the most frequently used API backwards compatible.

Right, thanks for your input – I'll probably add a couple more comments here as soon as I have the time to work on it. This is much appreciated.

Sylvester Keil
Owner

Can you explain what would be the advantage of using alias_method here?

I actually used alias specifically because it is a keyword and lexically scoped. I find the argument that it gives you more flexibility unconvincing: by the same logic wouldn't I have to use define_method over def everywhere? In Matz' book the main rationale for alias_method is described as providing a way of using alias with dynamic names, i.e. mostly for meta-programming.

I just follow the convention which prefers alias_method, all the commits are open for discussion. I just made this replacement quickly using a regex.

Sylvester Keil
Owner

The *.output files should stay in the gitignore and Rakefile – although I hope we won't have to debug the grammar again. They should not go into the repository or the gem though.

Good call on including the generated Ruby code. The advantages you list are all true, plus testing for JRuby will be much easier.

Ok, the output thing should stay.

Daniel Mendler minad closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 8, 2013
  1. Daniel Mendler

    minor Rakefile cleanup

    minad authored
    - don't require yard
    - don't require bundler
  2. Daniel Mendler

    don't require minitest-colorize for tests

    minad authored
    - also removed trailing whitespace
  3. Daniel Mendler

    lazy loading of multi json

    minad authored
  4. Daniel Mendler

    add the generated parsers to the repository

    minad authored
    - This needs discussion!
    - I think it is usually done like this in other projects
    - Since the gem also includes the generated file, it is better like this. And then one
      also sees if something really changes in the parser.
    - If someone starts developing, he doesn't need racc at first
  5. Daniel Mendler
  6. Daniel Mendler
  7. Daniel Mendler
  8. Daniel Mendler
  9. Daniel Mendler

    store type as string

    minad authored
  10. Daniel Mendler

    use alias_method instead of alias

    minad authored
    See http://stackoverflow.com/questions/4763121/should-i-use-alias-or-alias-method
    
    I also find the use of the aliases inflationary. A lot of them could be removed imho.
  11. Daniel Mendler
  12. Daniel Mendler
Something went wrong with that request. Please try again.