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

docs: Added additional docs for executeUpdate #2269

Merged
merged 14 commits into from
Aug 29, 2019

Conversation

danielgsims
Copy link
Contributor

  • Added notes that executeUpdate should be used for insert, update
    and delete statements, which should help quick searching of the
    docs.
  • Added an example update using a struct
  • Added links in the docs to DML guide

  * Added notes that executeUpdate should be used for insert, update
    and delete statements, which should help quick searching of the
    docs.
  * Added an example update using a struct
  * Added links in the docs to DML guide
@danielgsims danielgsims requested a review from jdpedrie as a code owner August 25, 2019 23:59
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 25, 2019
@danielgsims
Copy link
Contributor Author

Hey @jsimonweb - based on your PR in #2121, we wanted to improve our docs and snippets for our Spanner classes. I've added links to the DML guide, added notes that users can perform INSERT, UPDATE and DELETE statements via our Transaction::executeUpdate method, and included a struct example. Do you feel like this sufficiently improves the docs, or do you have any other suggestions? Thanks!

Copy link
Contributor

@jdpedrie jdpedrie left a comment

Choose a reason for hiding this comment

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

Did you verify that the struct example works against a real database schema and data?

Spanner/src/Transaction.php Outdated Show resolved Hide resolved
* rows).
* rows). For example, DML supports INSERT, UPDATE and DELETE statements. For
* more on DML syntax, visit the DML syntax guide.
* {@see https://cloud.google.com/spanner/docs/dml-syntax}
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this down near line 363. URLs in the doc text should be formatted as markdown.

Spanner/src/Transaction.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
* @codingStandardsIgnoreStart
*
Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line.

Spanner/tests/Snippet/TransactionTest.php Outdated Show resolved Hide resolved
Argument::withEntry('params', $expectedParams),
Argument::withEntry(
'paramTypes',
Argument::withEntry('post', Argument::withEntry('structType', $expectedStructData))
Copy link
Contributor

Choose a reason for hiding this comment

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

you can assert on nested array data? cool! TIL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! I found an example someone in phpspec, I'll send it to you if I can find it again.

Spanner/src/Transaction.php Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #2269 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #2269   +/-   ##
========================================
  Coverage      92.6%   92.6%           
  Complexity     4467    4467           
========================================
  Files           307     307           
  Lines         13323   13323           
========================================
  Hits          12338   12338           
  Misses          985     985
Impacted Files Coverage Δ Complexity Δ
Spanner/src/Transaction.php 100% <ø> (ø) 24 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92a5dd0...aa7f289. Read the comment docs.

@danielgsims
Copy link
Contributor Author

Did you verify that the struct example works against a real database schema and data?

This is specifically what I ran and tested against a real dataset:

        $database->runTransaction(function (Transaction $t) use ($spanner) {
            $nameValue = (new StructValue)
                ->add('Title', 'New Title')
                ->add('Content', 'Lorem Ipsum');
            $nameType = (new StructType)
                ->add('Title', Database::TYPE_STRING)
                ->add('Content', Database::TYPE_STRING);

            $rowCount = $t->executeUpdate(
                "UPDATE Posts SET Title = 'New Title' "
                . "WHERE STRUCT<Title STRING, Content STRING>(Title, Content) "
                . "= @post",
                [
                    'parameters' => [
                        'post' => $nameValue
                    ],
                    'types' => [
                        'post' => $nameType
                    ]
                ]);
            $t->commit();
            printf('Updated %d row(s).' . PHP_EOL, $rowCount);
        });

Copy link
Contributor

@dwsupplee dwsupplee left a comment

Choose a reason for hiding this comment

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

One small nit, otherwise looks nice.

* "WHERE STRUCT<Title STRING, Content STRING>(Title, Content) = @post";
*
* $postValue = new StructValue();
* $postValue->add('Title', 'Updated Title');
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a pretty nice opportunity to use the fluent interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure! How does this look? Would you rather go all-the-way with (new StructValue())->add()->add()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants