graphile / migrate

Opinionated SQL-powered productive roll-forward migration tool for PostgreSQL.
MIT License
751 stars 58 forks source link

allow spaces / tabs before --!include fixtures #207

Closed tsnobip closed 9 months ago

tsnobip commented 9 months ago

Description

The new fixture feature is really cool, but I hit an issue where my code was not being included and I was totally clueless!

Actually, the only issue was that I had a space before --!include...

That's why I suggest to relax the regex so it allows spaces/tabs before the pattern.

To be honest I would even relax it more so it gets triggered even when you forget the .sql extension so it raises an error instead of silently ignoring it as it does today.

Performance impact

none

Security impact

none

Checklist

benjie commented 9 months ago

I'm not happy doing that, any line prefixed --! is so powerful it should be entered very carefully. We shouldn't make an exception for just this one place, and the file splitting comments should definitely start at the beginning of the line, so I'm going to close this as "wontfix".

However, the filename matching is a good point, so if you want to raise a separate PR for that which validates the file in JS-land rather than regex-land, I'd be happy with that!