Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Added metadata for oracle #5032

Closed
wants to merge 1 commit into from
Closed

Added metadata for oracle #5032

wants to merge 1 commit into from

Conversation

tux-rampage
Copy link
Contributor

This commit adds a metadata source for Oracle as requested in #3956

This is missing the code for resolving triggers at the moment, but it's better than having no metadata source at all.
I've not been able to implement a Test case, yet.

It would be great if someone could tackle the missing items due to the lack of time from my side.

/**
* Constraint type
*
* @param string $type
Copy link
Contributor

Choose a reason for hiding this comment

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

double space after @param to consistency with #2419 and #2422

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but this isn't listed in the coding standards: http://framework.zend.com/wiki/display/ZFDEV2/Coding+Standards
Or did I miss it?

[edit: corrected url]

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but it just for consistency with >6000 files other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, but there are more important things than an additional whitespace after an annotation tag I guess. The time of the people that are comitting is valuable and most of them don't get paid for their effort. I'd rather like to see their resources spend in improving the frameworks code instead of changing unimportant stuff which is not even worth to be mentioned in the coding standards ...
No offense, but this looks a bit pedantic to me.

Copy link
Member

Choose a reason for hiding this comment

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

@tux-rampage we're all on the same boat. Making the framework better and trying to respect any possible aesthetic betterment is perfectly ok.

@samsonasik is ALSO putting his own free unpaid time into this. You are free to skip the CS comments if you think so, but please don't tell reviewers to not review, since they also put a lot of effort in it ;)

Copy link
Member

Choose a reason for hiding this comment

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

@samsonasik
Sorry, but I have to agree with @tux-rampage. This is not defined in the coding standards.
And if I look at the Zend\Db\Sql\Ddl classes (only one example), it is not fair to make these hard requirements.

Please do not get me wrong, I want the same: a better framework and a good documentation (incl. DocBlocks). But the additional whitespace is not in the coding standards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ocramius I don't complain about the review, which is highly encouraged and thanks for doing so. But complaining about a missing white space, which is not even required by CS, is imho a bit too much.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tux-rampage @froschdesign it's up to you to agree with me or not. I just make a suggestion for consistencies. It not be a problem for me if it merged without my change suggestion :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samsonasik Thanks for clarification. It sounded a lot like a must have requirement.

@tux-rampage
Copy link
Contributor Author

@ralphschindler

Could we merge this into the development tree?
What test cases should a Unit test for this metadata source cover? Imho this would be more an integration instead of an unit test.

@ralphschindler
Copy link
Member

Yep, I've been working through the DB pull requests, and this one is on my list. I should be able to get to the Oracle specific ones today.

@tux-rampage
Copy link
Contributor Author

cool, thanks. 👍

ralphschindler pushed a commit that referenced this pull request Dec 2, 2013
Merge branch 'zf3956' of git://github.com/tux-rampage/zf2 into tux-rampage-zf3956

* 'zf3956' of git://github.com/tux-rampage/zf2:
  Added metadata for oracle See issue #3956
ralphschindler pushed a commit that referenced this pull request Dec 2, 2013
Merge branch 'tux-rampage-zf3956' into develop

* tux-rampage-zf3956:
  Added metadata for oracle See issue #3956
@ralphschindler ralphschindler self-assigned this Mar 11, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants