Skip to content

Conversation

@flash-gordon
Copy link
Contributor

Ok, here we go :)

Keeping my (outdated) promise from #31 I refactored package.rb.

Best regards,
NS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice separation of responsibility. I like the way you got rid of the If/else

@jgebal
Copy link
Contributor

jgebal commented Jul 8, 2015

@javornikolov, will you be able to review/approve?

@javornikolov
Copy link
Collaborator

Thank you @flash-gordon for the refactoring and thanks @jgebal for your review!

I can take a bit more detailed look by the end of the week, at first glimpse it looks good to me.

@flash-gordon flash-gordon force-pushed the refactor-package branch 2 times, most recently from 8054e3e to 963311d Compare July 9, 2015 22:35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgebal is it better now? Or did I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

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

@flash-gordon Thanks, I'll review it over the weekend - too tired to think clear now.

@jgebal
Copy link
Contributor

jgebal commented Jul 12, 2015

Except the 2 comments added, the PR is really OK from my perspective.

@javornikolov
Copy link
Collaborator

OK from my side too.
I started wondering about the cases when we have low-case letters in the database object names (e.g. "MyPackage"), but that's a separate topic and is anyway an obscure practice.

@jgebal
Copy link
Contributor

jgebal commented Jul 15, 2015

I'd give it a go with this pull request.
Case-sensitivity in Oracle is in general a bad practice I think. Just because you can use it, doesn't man that you should.
Since Ruby uses low case for variables, and Oracle, well - depends on company standards :), I'd allow the 'whatever case' approach for sting-based.
On the other hand, the framework is using symbols, not strings wherever possible. That seems a better approach.
@flash-gordon - any particular reason why you want to reference package objects by using the below syntax?
[code]
package_name['string_VAR_name']
[code]
would it be enough to use:
[code]
package_name[:symbol_var_name]
[code]

* Split Package.find into smaller parts
* Improve Package#method_missing readability
@flash-gordon
Copy link
Contributor Author

@jgebal no, I'm fine with case-insensitive approach, just updated the PR. Also we can add string/symbol semantics for case-sensitivity (something like Sequel's implementation for column names).

@jgebal
Copy link
Contributor

jgebal commented Jul 15, 2015

@javornikolov All tests are passing, Can you can merge and close this one?

@javornikolov
Copy link
Collaborator

Great, I'm merging. Thank you @flash-gordon for that contribution and thank you @jgebal for reviewing it!

javornikolov added a commit that referenced this pull request Jul 16, 2015
Accessing package objects via #[]
@javornikolov javornikolov merged commit 4eb5861 into rsim:master Jul 16, 2015
@javornikolov javornikolov modified the milestone: 0.6.0 Mar 13, 2016
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.

3 participants