Skip to content
This repository has been archived by the owner on Dec 12, 2019. It is now read-only.

Create gem #2

Merged
merged 7 commits into from
Oct 18, 2013
Merged

Create gem #2

merged 7 commits into from
Oct 18, 2013

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Sep 27, 2013

As discussed with @Fryguy this pull request represents the work I was able to do according to the instructions on building a gem.

It also includes a fix to provide default options if the user does not provide a YAML file,
which could happen if they logged in manually before using this gem.

EDIT @Fryguy: Removed link because it's not publicly accesbile anyway.

The Code Climate README.md changes were already made too.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling fa4e295 on jvlcek:create_gem into * on ManageIQ:master*.

@Fryguy
Copy link
Member

Fryguy commented Sep 27, 2013

The spec is failing on JRuby because of how the tempfile creator is creating the file. Since the code is running on travis, we probably don't have access to write to a directory. I have 2 suggestions.

  1. Remove JRuby support from the travis.yml. It also seems there's a problem with Rubyinius 1.8, so remove that as well. Since we are using LinuxAdmin for shelling out, I don't think those two will work anyway.
  2. Change the interface to ruby_bugzilla to not require a file. I think the API for ruby_bugzilla should have parameters to login that the user can specify. In addition, there could be class accessors for setting it once in code. We may want to remove the class accessors in favor of instance methods, but maybe that can come later.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0225ece on jvlcek:create_gem into * on ManageIQ:master*.

@jvlcek
Copy link
Member Author

jvlcek commented Sep 30, 2013

@Fryguy and @jrafanie

I'd first like to implement your suggestion #1 "Removed JRuby and Rubyinius 1.8 support from the travis.yml."

I also like your suggestion #2 "Changing the interface to ruby_bugzilla to not require a file." but I would rather tackle that as a separate issue and for now focus on adding "modify" support.

@jrafanie
Copy link
Member

jrafanie commented Oct 1, 2013

@jvlcek It says "Removed JRuby and Rubinius 1.9 support from the travis.yml" in both the commit message, the yaml file and this pull request's description but @Fryguy mentioned that it was JRuby and Rubinius 1.8 that was a problem. Can you confirm what version is the issue and make any corrections to the travis.yml and commit messages to make to very clear which ruby implementations are being disabled for now.

Additionally, please open a ticket to add support for those implementations if it's easy to do.

Finally, I think we need to bump the minor or patch version number to abide by semantic versioning since you're adding a dependency for coveralls, http://semver.org/

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 29c576f on jvlcek:create_gem into * on ManageIQ:master*.

@jvlcek
Copy link
Member Author

jvlcek commented Oct 1, 2013

@jrafanie, I've corrected the commit message and this pull request's description to correctly identify Rubinius 1.8 that.

I've updated the minor version for the coveralls dependency.

I have the functionality ready for Jason's API change suggestinos and will post it in a pull request once this is merged.

Thank you!
JoeV

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e5c0e77 on jvlcek:create_gem into * on ManageIQ:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0bb5f8e on jvlcek:create_gem into * on ManageIQ:master*.

@@ -7,18 +7,41 @@ class RubyBugzilla
COOKIES_FILE = File.expand_path('~') + '/.bugzillacookies'
CREDS_FILE = File.expand_path('~') + '/.bugzilla_credentials.yaml'

@@username = nil
@@password = nil
Copy link
Member

Choose a reason for hiding this comment

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

Don't use class variables. As much as you think they make sense, in this case, they don't, unfortauntely. Prefer class-instance variables. So, you don't need these two lines at all, and in the next 4 methods, just use single-@ instead of double-@

Copy link
Member Author

Choose a reason for hiding this comment

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

D'oh! Of course... Thanks for catching that.

@jvlcek
Copy link
Member Author

jvlcek commented Oct 10, 2013

@Fryguy Thank you again for the feedback. I believe I've address the issues you've raised. I slightly changed the way you suggested to code the credentials method by allowing for username and password to be input parameters to that method. I think it might be valuable to the user of the API.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6a9fac4 on jvlcek:create_gem into * on ManageIQ:master*.

@jvlcek
Copy link
Member Author

jvlcek commented Oct 17, 2013

@Fryguy and @jrafanie Now that we are through the blocker bugs can you please finalize this and merge if my latest updates satisfactorily address all the issue raised.

Fryguy added a commit that referenced this pull request Oct 18, 2013
@Fryguy Fryguy merged commit 8400252 into ManageIQ:master Oct 18, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants