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

Library Refactor #7

Closed
wants to merge 22 commits into from
Closed

Library Refactor #7

wants to merge 22 commits into from

Conversation

doomspork
Copy link

Hi @cew821,

First off, thanks for putting this gem together! I've been using the it for a project of mine and I thought I'd spend some time contributing back; Apologizes in advance for the large PR.

An overview of the changes:

  • Updated development dependencies to latest versions.
  • Replaced double quotes with single quotes to be consistent across files.
  • Added Gemfile.lock, .ruby-version, and .ruby-gemset to .gitignore, standard in gems.
  • Added additional files to the .gitignore for other environments and editors.
  • Refactored each Green Button model and test into their own file.
  • Namespaced objects according to their folder structure.
  • Removed gem binaries from git repo — git tagging should be used.
  • Minor README changes.
  • Added test coverage reporting.
  • Replaced .load_xml_from_file and .load_xml_from_web with .load; The project already made use of open-uri so I leveraged that existing dependency to consolidate the public API into a single method.

These changes have increased the Code Climate score from 3.6 to 4.0 and increased the test coverage by 2%.

Code Climate

In addition to this work I'm continuing to look at how to add test coverage and the possibility of replacing Nokogiri with a SAX parser for better large file performance.

Let me know if you have any thoughts, questions, or concerns with these changes. I can always create a separate gem but I think the community benefits from a consolidated effort.

Thanks again!

@cew821
Copy link
Owner

cew821 commented May 11, 2015

Thank you so much for taking the time to do this! I no longer work on a project that uses Green Button, but I'm glad to see that it's still of use to someone. I will review this soon -- sometime in the next few days.

If you are interested, I would also definitely be open to adding you as a collaborator to this gem (given that I'm not active in this space now).

@doomspork
Copy link
Author

That would be awesome @cew821 — I'm just ramping up in the space and am currently leading a large GB-centric project.

@erichulburd
Copy link
Collaborator

Charles, If you aren't active in this space any more, could you add me as a
contributor/collaborator to the repository and gem as well? I'd definitely
like to see it maintained and respond to these sorts of issues, especially
if you aren't available.

Thanks,

Eric

On Mon, May 11, 2015 at 12:11 PM, Charles Worthington <
notifications@github.com> wrote:

Thank you so much for taking the time to do this! I no longer work on a
project that uses Green Button, but I'm glad to see that it's still of use
to someone. I will review this soon -- sometime in the next few days.

If you are interested, I would also definitely be open to adding you as a
collaborator to this gem (given that I'm not active in this space now).


Reply to this email directly or view it on GitHub
#7 (comment).

Eric Hulburd

Oroeco.com http://www.oroeco.com/
Twitter: @oyecalifornia https://twitter.com/oyecalifornia
Ciudad de México

@doomspork
Copy link
Author

@cew821 I'm inclined to close this PR and work from my own repo going forward. Would you be willing to give me access to the gem on Rubygems?

@cew821
Copy link
Owner

cew821 commented May 12, 2015

Hi @Arbolista -- I tried to ping you on this but your screen name had changed. I would love your eyes on this PR.

@doomspork I'm open to that but would like @Arbolista's take as well as Eric contributed a lot to the early work on the gem. Hopefully you two could collaborate on the project going forward and come to a consensus on the best path forward!

@cew821
Copy link
Owner

cew821 commented May 12, 2015

I'm also curious if either of you have checked out the ce-greenbutton gem -- seems to be under active development and was built based on this top coder challenge. I haven't looked into it but I wonder if there are any ideas in it worth using.

@doomspork
Copy link
Author

@cew821 I had not but I can't find the source, do you where it can be located?

@cew821
Copy link
Owner

cew821 commented May 12, 2015

I don't think it's on github so you'll have to just unpack the gem file itself once you $ gem install ce-greenbutton.

@erichulburd
Copy link
Collaborator

I'm all for working with whoever wants to contribute and keep the repo and
gem active and maintain a community (as opposed to splitting our efforts).

So if Sean you are willing to add me as a contributor on GitHub and
collaborator on the gem, I'm more than happy to support the repo moving to
your GitHub account.

On Mon, May 11, 2015 at 5:24 PM, Charles Worthington <
notifications@github.com> wrote:

Hi @Arbolista https://github.com/Arbolista -- I tried to ping you on
this but your screen name had changed. I would love your eyes on this PR.

@doomspork https://github.com/doomspork I'm open to that but would like
@Arbolista https://github.com/Arbolista's take as well as Eric
contributed a lot to the early work on the gem. Hopefully you two could
collaborate on the project going forward and come to a consensus on the
best path forward!


Reply to this email directly or view it on GitHub
#7 (comment).

Eric Hulburd

Oroeco.com http://www.oroeco.com/
Twitter: @oyecalifornia https://twitter.com/oyecalifornia
Ciudad de México

@doomspork
Copy link
Author

@Arbolista I'm open to that if @cew821 it's onboard with adding us to the gem.

@doomspork
Copy link
Author

@Arbolista, you've been added to my repo; let's make use of Issues to discuss some next steps?

@erichulburd
Copy link
Collaborator

@doomspork great. I've not used it extensively recently, but have been trying monitor issues. If you have a feature/ but you want assistance with, let me know.

@doomspork
Copy link
Author

@Arbolista right now I'm trying to get up to speed on the current functionality to make sure nothing is missing. After that I'd love to improve the test coverage a bit and then evaluate whether SAX parsing makes sense.

@cew821 anything you can share about the current status would be greatly appreciated.

@doomspork
Copy link
Author

@cew821, @Arbolista — I'm going to go ahead and create a separate gem, doesn't seem like this is going to go anywhere.

@doomspork doomspork closed this May 27, 2015
@cew821
Copy link
Owner

cew821 commented May 27, 2015

Hey @doomspork still happy to have you take over the current gem, though not exactly sure on the mechanics of transferring. Did you wind up continuing dev in your own fork?

@doomspork
Copy link
Author

Hey @cew821, I'm still working on the gem through my own fork at this time. We're already 2 weeks into a 6 week development cycle that needed this library, hence the need to change directions and evaluate publishing our own gem.

I found this on the RubyGem support forums: Adding Another Owner.

@cew821
Copy link
Owner

cew821 commented Jun 1, 2015

What email address would you like me to use for adding you to the gem? I'll add you tonight!

@doomspork
Copy link
Author

Hi @cew821, you can use seancallan@gmail.com. That one should be the one associated with my other gems et al. If you or @Arbolista want to chat RE: greenbutton, feel free to reach out to me via the above address; always interested in networking with folks doing this kind of stuff.

Thanks again!

@cew821
Copy link
Owner

cew821 commented Jun 3, 2015

Done! while I don't work much with Green Button any more, I'm glad the work we did on this gem will live on!

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.

None yet

3 participants