search

rowland / fb

Firebird Extension Library for Ruby
64 stars 35 forks source link

clean codebase #39

Closed vizcay closed 9 years ago

vizcay commented 9 years ago

Hi Rowland, after my first incursion with this gem (and a ruby c extension) I'm thinking a few ways to improve the gem aesthetically speaking to encourage collaboration so it's no so intimidating to newcomers:

So, the codebase will remain the same.. what do you think? I can have a pull requests in the near future if you agree.

By the way, are you going to bump version to 0.8 and push to rubygems master?

mariuz commented 9 years ago

please send a pull request for each issue and then we can discuss :+1:

vizcay commented 9 years ago

Mariuz: I want to know the repo owner thoughts on this before commiting any work time.

rowland commented 9 years ago

I certainly understand how intimidating a new codebase can be when the files are large. However, it may prove tricky to slice up fb.c into logical chunks that are superior to the status quo. You could say I already did it when I ported the code to Go (https://github.com/rowland/go-fb), but the core files still turned out pretty large. I'm not convinced it's a particularly good investment of time.

I find hard tabs to be a good match for C and C-like languages. I aim for sanity with 4-space hard tabs for C, 2-space soft tabs for Ruby, in line with the Rails standard.

I have no objection to the removal of trailing whitespace, but I avoid changing lines unrelated to the purpose of a commit, so a commit should contain only whitespace/formatting changes or code changes, but not both.

Most of the dead code relates to global transactions, which was non-reentrant and memory leak prone. It hasn't been missed and can be removed.

The README is dated, but worth preserving, perhaps with a different name. A README.md would look good on Github.

I haven't pushed a new version to rubygems because I haven't had a chance to verify backwards compatibility or even compatibility with activerecord-fb-adapter. Apparently BigDecimal.new("2.5").to_s yields "0.25E1" which could break some code in the wild. Perhaps I'm missing something, but I believe more testing is needed.

vizcay commented 9 years ago

I believe refactoring the big file will be useful and hard tabs are evil, but is your repo, and forking and wasting efforts for such a small thing is not worth it. I will just state that I disagree with the decision.

I then will fix the trailing whitespaces, make the README look better with markdown and remove the chunks of code that are commented.

About your concern with BigDecimal, I can't claim that there will not be problems ahead, legacy code that uses the fb gem directly will be expecting a Float for a decimal or numeric column and may break, but because of that is that we talked about bumping to 0.8 and warning everyone. About the activerecord adapter, rails automatically casts the decimal columns to a Bigdecimal directly so I that should't be such an issue, but there may be another unrelated problems lurking.. who knows. But still I believe the patch is in the good direction: a decimal is a decimal and should be of fixed precision, currently some decimal(18,2) values get returned from the database as "xxx.xx000000001" and that for my is a serious issue when handling currency related stuff.

mariuz commented 9 years ago

this i guess can be closed

i started with remove old commented code , and the rest can be contributed with pull requests on the issue with better readme