Wrap included files with include comment

astephensen commented 7 months ago

Description

This PR will wrap included files with comments, as first described in https://github.com/graphile/migrate/issues/208.

This also facilitates removing the included content when performing an uncommit.

Have opened as a draft as I'm open to any and all feedback!

Pre-Commit

current.sql

select 'foo';
--!include functions/hello.sql

fixtures/functions/hello.sql

create or replace function public.hello() returns text as $$
  select 'Hello, world!';
$$ language sql;

Committed

migrations/000001-world.sql

--! Previous: -
--! Hash: sha1:2d5fea0f4300a51ebf5c9d2416def4f2d22563d3
--! Message: World

select 'foo';
--! Included functions/hello.sql
create or replace function public.hello() returns text as $$
  select 'Hello, world!';
$$ language sql;
--! EndIncluded functions/hello.sql

Uncommitted

current.sql

--! Message: World

select 'foo';
--!include functions/hello.sql

Implementation

The method I implemented is similar to what @jcgsville described in the original issue, with a few little changes.

The colon was dropped between the header and the filename — I was encountering issues with --! Include: being treated as an actual header if the first statement was an include. Figured dropping the colon helps distinguish this as a file include, but I'm open to suggestions on how to fix this!
The filename is also added after the --! EndIncluded — This allows a simple regex back reference to be used to delete the included file (if the filename wasn't added, nested includes could be trickier to remove).

The main downside of this approach is the original formatting of the --!include is not preseved, i.e. if the include is written as --! include ... the migration will not restore the additional whitespace when uncommitting.

Notes

I had to run prettier across a lot of files that I didn't touch — not sure if this was missed in a previous PR or if I didn't have it configured correctly locally, but it was causing the tests to fail.

Performance impact

unknown - likely minimal.

Security impact

unknown - likely none.

Checklist

[x] My code matches the project's code style and yarn lint:fix passes.
[x] I've added tests for the new feature, and yarn test passes.
[x] I have detailed the new feature in the relevant documentation.
[x] ~I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).~ - Does not exist
[x] ~If this is a breaking change I've explained why.~ - Not a breaking change

jcgsville commented 7 months ago

Thanks for taking a stab at this, @astephensen 🙂

benjie commented 6 months ago

I had to run prettier across a lot of files that I didn't touch — not sure if this was missed in a previous PR or if I didn't have it configured correctly locally, but it was causing the tests to fail.

I think you must have it misconfigured (older prettier version?) - use yarn && yarn lint:fix to fix formatting issues.

astephensen commented 6 months ago

Thanks for the review @benjie, I will address those comments!

graphile / migrate