Skip to content

Conversation

@gacha
Copy link

@gacha gacha commented Oct 19, 2015

Now it is possible to determine if a field has default arguments.

Reciepe chef/centos-6.6 doesn't exists any more, so updated it to 6.7.
New CentOS image doesn't permit password from dictionary, so changed it
to something else.

@jgebal
Copy link
Contributor

jgebal commented Oct 19, 2015

Nice work.
Can you add a unit test for the defaulted columns?

@gacha
Copy link
Author

gacha commented Oct 19, 2015

will do

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since which Oracle release do we have this defaulted attribute? What would happen if we run against e.g. 10g?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, on 10g there's no such column, so I will do like in subprogram_id_column.

@javornikolov
Copy link
Collaborator

@gacha : would you mind splitting the test VM setup changes into a separate pull request (seems an independent topic). Thank you!

@gacha gacha force-pushed the feature/has-default-value branch from bfe8a4a to c7062e5 Compare January 18, 2016 15:23
@gacha
Copy link
Author

gacha commented Jan 18, 2016

Finally got time to finish this, removed the Vagrant change because someone already did that, also added tests.

Also I had problems running tests as it's described in Readme, I had to add DATABASE_USE_TNS=NO to anything happen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to specify the schema name here hr.?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering about a better name - defaulted_value sounds more like the value itself (21), while we expect the whole expression/clause here.

@javornikolov
Copy link
Collaborator

Thank you @gacha, it looks great! I just posted a few cosmetic remarks which I think is good to sort out and we can merge the PR.

@gacha
Copy link
Author

gacha commented Jan 21, 2016

Anything else?

@javornikolov
Copy link
Collaborator

Thanks, it looks good to me 👍 @gacha, could you please just squash the two commits into a single one before we merge it.

Now it is possible to determine if a field has default arguments.
@gacha gacha force-pushed the feature/has-default-value branch from 7bdc6be to 110b67c Compare January 22, 2016 07:29
@gacha
Copy link
Author

gacha commented Jan 22, 2016

Done

@javornikolov
Copy link
Collaborator

Nice, I'm merging it... @gacha, thank you very much for your contribution!

javornikolov added a commit that referenced this pull request Jan 22, 2016
New procedure argument metadata option 'defaulted'
@javornikolov javornikolov merged commit c1b33fc into rsim:master Jan 22, 2016
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants