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

ALTER TABLE statement as multiple pages #950

Merged
merged 1 commit into from Dec 31, 2016
Merged

ALTER TABLE statement as multiple pages #950

merged 1 commit into from Dec 31, 2016

Conversation

sploiselle
Copy link
Contributor

@sploiselle sploiselle commented Dec 28, 2016

Closes #874
Closes #738
Closes #737
Closes #736
Closes #735
Closes #705
Closes #252

#934 still open


This change is Reviewable

@jseldess
Copy link
Contributor

Looks great overall. I'll review each new subcommand page in a bit, but just want to send over some initial, higher-level feedback.


Review status: 0 of 14 files reviewed at latest revision, 3 unresolved discussions.


alter-table.md, line 7 at r1 (raw file):

---

The `ALTER TABLE` [statement](sql-statements.html) applies a schema change to a table.

Since privileges and diagrams are covered on the pages for subcommands, I don't think we need to include those sections here. Ideally, this page would just give a basic intro and redirect users to the page for the specific subcommand they're interested in. Thoughts?


alter-table.md, line 21 at r1 (raw file):

The user must have the `CREATE` [privilege](privileges.html) on the table. 

## Subcommands

Can you add RENAME COLUMN and RENAME TABLE as well? This was one problem Alex was having in #874. He expected to find rename column listed here.

Are there any other subcommands to add?


sql-statements.md, line 11 at r1 (raw file):

Statement | Usage 
----------|------------
[`ADD COLUMN`](add-column.html) | Add columns to a table.

Please add these to the SQL Statements sidenav as well.


Comments from Reviewable

@jseldess
Copy link
Contributor

@sploiselle, added some feedback on the pages for specific subcommands. Again, looks excellent overall. Thanks!


Review status: 0 of 14 files reviewed at latest revision, 9 unresolved discussions.


add-constraint.md, line 15 at r1 (raw file):

{{site.data.alerts.callout_info}}
The <a href="default-value.html">Default</a> constraint is managed through <a href="alter-column.html"><code>ALTER COLUMN</code></a>.<br/><br/>
The <a href="primary-key.html">Primary Key</a> constraint can only be applied through <a href="create-table.html"><code>CREATE TABLE</code></a>.

big nit: I think these can just be two contiguous sentences.


add-constraint.md, line 34 at r1 (raw file):

| `table_name` | The name of the table containing the column you want to constrain. |
| `name` | The name of the constraint, which must be unique to its table and follow these [identifier rules](keywords-and-identifiers.html#identifiers). |
| `constraint_elem` | The [Check](check.html), [Foreign Keys](foreign-key.html), [Unique](unique.html) constraint you want to add. <br/><br/>Adding/changing a Default constraint is done through `ALTER COLUMN`. <br/><br/>Adding/changing the table's Primary Key is not supported through `ALTER TABLE`; it can only be specified during [table creation](create-table.html#create-a-table-primary-key-defined). |

, or Unique constraint...

Also, let's make ALTER COLUMN a link to that page.


add-constraint.md, line 36 at r1 (raw file):

| `constraint_elem` | The [Check](check.html), [Foreign Keys](foreign-key.html), [Unique](unique.html) constraint you want to add. <br/><br/>Adding/changing a Default constraint is done through `ALTER COLUMN`. <br/><br/>Adding/changing the table's Primary Key is not supported through `ALTER TABLE`; it can only be specified during [table creation](create-table.html#create-a-table-primary-key-defined). |

## Examples

These example are clear, but it might be more helpful, especially to users not experienced with constraints, to fill in the context a bit more. What do you think?


alter-column.md, line 29 at r1 (raw file):

| `a_expr` | The new Default value you want to use. |

## Examples

Same command as above. These example are clear, but it might be more helpful, especially to novice users, to fill in the context a bit more. What do you think?


drop-column.md, line 28 at r1 (raw file):

| `RESTRICT` | *(Default)* Do not drop the column if any objects (such as [views](views.html)) depend on it. |

## Examples

Same feedback as for examples on other pages. Perhaps we should provide a bit more context for each of these.


drop-constraint.md, line 41 at r1 (raw file):

~~~
~~~ sql
> ALTER TABLE orders DROP CONSTRAINT fk_customer_ref_customers;

Can you show the response to this statement as well?


Comments from Reviewable

@sploiselle
Copy link
Contributor Author

Review status: 0 of 15 files reviewed at latest revision, 9 unresolved discussions.


add-constraint.md, line 15 at r1 (raw file):

Previously, jseldess wrote…

big nit: I think these can just be two contiguous sentences.

Done.


add-constraint.md, line 34 at r1 (raw file):

Previously, jseldess wrote…

, or Unique constraint...

Also, let's make ALTER COLUMN a link to that page.

Done.


add-constraint.md, line 36 at r1 (raw file):

Previously, jseldess wrote…

These example are clear, but it might be more helpful, especially to users not experienced with constraints, to fill in the context a bit more. What do you think?

Done.


alter-column.md, line 29 at r1 (raw file):

Previously, jseldess wrote…

Same command as above. These example are clear, but it might be more helpful, especially to novice users, to fill in the context a bit more. What do you think?

Done.


alter-table.md, line 7 at r1 (raw file):

Previously, jseldess wrote…

Since privileges and diagrams are covered on the pages for subcommands, I don't think we need to include those sections here. Ideally, this page would just give a basic intro and redirect users to the page for the specific subcommand they're interested in. Thoughts?

Done.


alter-table.md, line 21 at r1 (raw file):

Previously, jseldess wrote…

Can you add RENAME COLUMN and RENAME TABLE as well? This was one problem Alex was having in #874. He expected to find rename column listed here.

Are there any other subcommands to add?

Had this change stashed. Thanks for catching it. I did do some digging since you asked and found ALTER TABLE...SPLIT AT which has some contention around whether or not it should be documented at all. Throwing it in the table with a brief description.


drop-column.md, line 28 at r1 (raw file):

Previously, jseldess wrote…

Same feedback as for examples on other pages. Perhaps we should provide a bit more context for each of these.

Done.


drop-constraint.md, line 41 at r1 (raw file):

Previously, jseldess wrote…

Can you show the response to this statement as well?

Done.


sql-statements.md, line 11 at r1 (raw file):

Previously, jseldess wrote…

Please add these to the SQL Statements sidenav as well.

Done.


Comments from Reviewable

@jseldess
Copy link
Contributor

:lgtm:, with one final nit.


Review status: 0 of 15 files reviewed at latest revision, 4 unresolved discussions.


alter-column.md, line 33 at r2 (raw file):

### Set or Change a Default Value

Setting the [Default value constraint](default-value.html) inserts the value when data's written to the table without explicitly defining the value for the column. If the column already has Default value, you can use this statement to change it.

"If the column already has a Default value..."


alter-column.md, line 35 at r2 (raw file):

Setting the [Default value constraint](default-value.html) inserts the value when data's written to the table without explicitly defining the value for the column. If the column already has Default value, you can use this statement to change it.

The below example inserts the boolean value `true` whenever you inserted data to the `subscriptions` table without defining a value for the `newsletter` column.

boolean -> Boolean

Also, "...whenever you insert data..."


Comments from Reviewable

@jseldess
Copy link
Contributor

Reviewed 8 of 14 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@sploiselle
Copy link
Contributor Author

Review status: 14 of 15 files reviewed at latest revision, 4 unresolved discussions.


alter-column.md, line 33 at r2 (raw file):

Previously, jseldess wrote…

"If the column already has a Default value..."

Done.


alter-column.md, line 35 at r2 (raw file):

Previously, jseldess wrote…

boolean -> Boolean

Also, "...whenever you insert data..."

Done.


Comments from Reviewable

@sploiselle
Copy link
Contributor Author

Thanks for the reviews over the holiday, @jseldess!

@sploiselle sploiselle merged commit 59cf871 into gh-pages Dec 31, 2016
@sploiselle sploiselle deleted the alter-table branch December 31, 2016 15:10
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.

None yet

3 participants