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

Fix extraneous white space in generated sql #20340

Merged
merged 1 commit into from
May 19, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 18, 2021

Overview

Fix extraneous white space in generated sql - this incorporates #20339 and #20338 and a bunch more change and leaves us with fairly attractive sql at the end.

Schema.tpl whitespace overhaul

Before

If I regenerate sql in search kit I get

  • trailing space when comment is not declared for a db table
  • spaces before arguments that don't exist
  • extra lines

image

After

Schema.tpl less readable but generated sql much better (seems like a fair price IMHO)

image

Technical Details

Note I tried regenerating civicrm.mysql with this & it looked good - jenkins can
pass judgement too...

I didn't include the regenerated sql in search_kit but I hit what seems to me to be
an unreleated regression in civix - ie search_kit generates
ROW_FORMAT=DYNAMIC

With my version (lastest from master) of civix I get ENGINE = INNODB but
not the ROW_FORMAT

Comments

If totten/civix#202 were merged we possibly would no longer have generated sql files but that got stalled on negotiating feedback 6 months ago & I don't think it should be brought back onto the front burner given other priorities - so this could be short-lived formatting but better to have it now IMHO

@civibot
Copy link

civibot bot commented May 18, 2021

(Standard links)

- trailing space when comment is not declared for a db table
- spaces before arguments that don't exist
- extra lines

Schema.tpl whitespace overhaul

Note I tried regenerating civicrm.mysql with this & it looked good - jenkins can
pass judgement too...

I also tried regenerating the sql in search_kit but I hit what seems to me to be
an unreleated regression - ie search_kit generates
 ROW_FORMAT=DYNAMIC

With my version (lastest from master) of civix I get ENGINE = INNODB but
not the ROW_FORMAT
@seamuslee001
Copy link
Contributor

This looks good to me merging

@seamuslee001 seamuslee001 merged commit 7399f09 into civicrm:master May 19, 2021
@seamuslee001 seamuslee001 deleted the schema2 branch May 19, 2021 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants