doctrine/sql-formatter

Improve formatting of MODIFY, CHANGE and AFTER

MaximilianKresse opened this issue · 4 comments

MaximilianKresse commented

I found some formatting problems in the current version 1.1.0.

1. MODIFY

Input

ALTER TABLE `table` MODIFY `id` INT(11) UNSIGNED NOT NULL

Result

ALTER TABLE
  `table` MODIFY `id` INT(11) UNSIGNED NOT NULL

Expected result

ALTER TABLE
  `table`
MODIFY
  `id` INT(11) UNSIGNED NOT NULL

2. CHANGE

Input

ALTER TABLE `table` CHANGE `id` `_id` BIGINT(20) UNSIGNED NULL

Result

ALTER TABLE
  `table` CHANGE `id` `_id` BIGINT(20) UNSIGNED NULL

Expected result

ALTER TABLE
  `table`
CHANGE
  `id` `_id` BIGINT(20) UNSIGNED NULL

3. AFTER

Input

ALTER TABLE `table` MODIFY `id` INT(11) UNSIGNED NOT NULL FIRST, CHANGE `name` `firstname` VARCHAR(20) COLLATE 'utf8mb4_unicode_ci' NOT NULL AFTER `id`, CHANGE `name2` `lastname` VARCHAR(30) COLLATE 'utf8mb4_unicode_ci' NOT NULL AFTER `firstname`, ADD `city` VARCHAR(20) COLLATE 'utf8mb4_unicode_ci' AFTER `lastname`

Result

ALTER TABLE
  `table` MODIFY `id` INT(11) UNSIGNED NOT NULL FIRST,
  CHANGE `name` `firstname` VARCHAR(20) COLLATE 'utf8mb4_unicode_ci' NOT NULL
AFTER
  `id`,
  CHANGE `name2` `lastname` VARCHAR(30) COLLATE 'utf8mb4_unicode_ci' NOT NULL
AFTER
  `firstname`,
ADD
  `city` VARCHAR(20) COLLATE 'utf8mb4_unicode_ci'
AFTER
  `lastname`

Expected result

ALTER TABLE
  `table`
MODIFY
  `id` INT(11) UNSIGNED NOT NULL FIRST,
CHANGE
  `name` `firstname` VARCHAR(20) COLLATE 'utf8mb4_unicode_ci' NOT NULL AFTER `id`,
CHANGE
  `name2` `lastname` VARCHAR(30) COLLATE 'utf8mb4_unicode_ci' NOT NULL AFTER `firstname`,
ADD
  `city` VARCHAR(20) COLLATE 'utf8mb4_unicode_ci' AFTER `lastname`
greg0ire commented

The formatting for AFTER does look a bit wrong, WDYT @goetas? If we were to accept this change, should it be considered a BC-break? It was added in jdorn/sql-formatter@2efc4bb4 with no explanation.

@MaximilianKresse you can already send a PR for MODIFY and CHANGE if you want to. Since they are not tested, I'd say there is no enforced contract and no BC-break…

goetas commented

To be honest I do not have strong opinion here. I think that changing the formatting should not be considered a BC break in any case.

greg0ire commented

Ok @MaximilianKresse you can add AFTER to your PR then I suppose. Be careful to fetch and reset to d529990, I rebased your PR on 1.1.x . This should be considered a fix and will ship in 1.1.1 right after we merge it.