cli ergonomics #172

psteinroe · 2025-01-21T16:39:13Z

goal of this pr is to gear the cli towards the use case we want to support initially: running a check command on migration file(s).

after a discussion with @juleswritescode, this is the plan:

We will have one single pgtoools check command. by default, it will scan everything for sql files and lint it. it can be filtered by path, e.g. pgtools check supabase/migrations to just lint the migration directory. We will have a migrations-dir config option that will enable extra filtering features for files within the directory, primarily --after and maybe --last-nth. Furthermore, we will also have --staged and --changed to easily lint just the relevant migrations in a CI environment or a pre-commit hook. The existing --include and --exclude filters will continue to work.

To enable --after, we will pass a --migrations-pattern.

how popular tools store migrations:

drizzle: migration.sql in a directory with the timestamp and the name. (link)
prisma: same as drizzle (link)
supabase: migrations dir with sql files
typeorm: stores migrations in ts files -> not supported for now

UPDATE: I removed --migrations-pattern again - we just try both and check if any yield an expected value. Added migration_dir as well as after :)

I initially tried to put it into get_files_to_process but that only looks at the inputs from the user and we need the traversal output to look at each file recursively. the right place for that is can_handle, which calls is_path_ignored from the workspace. the downside of my approach is that we check ignore patterns also for files, whereas before we just checked it for directories and symlinks.... an alternative would be a new workspace api aka is_migration_ignored, but that would conflict with is_path_ignored imo.

psteinroe · 2025-01-27T21:33:50Z

crates/pg_cli/src/execute/traverse.rs

+
+        let is_valid_file = self.fs.path_is_file(path)
+            && path
+                .extension()
+                .is_some_and(|ext| ext == "sql" || ext == "pg");
+
+        if self.fs.path_is_dir(path) || self.fs.path_is_symlink(path) || is_valid_file {


here is the relevant part in can_handle

psteinroe · 2025-01-27T21:35:14Z

crates/pg_workspace/src/workspace/server.rs

    /// Check whether a file is ignored in the top-level config `files.ignore`/`files.include`
    fn is_ignored(&self, path: &Path) -> bool {
        let file_name = path.file_name().and_then(|s| s.to_str());
        // Never ignore PGLSP's config file regardless `include`/`ignore`
        (file_name != Some(ConfigName::pglsp_toml())) &&
            // Apply top-level `include`/`ignore
-            (self.is_ignored_by_top_level_config(path))
+            (self.is_ignored_by_top_level_config(path) || self.is_ignored_by_migration_config(path))


is_ignored is called by is_path_ignored and we add the new check here.

very sweet!

psteinroe · 2025-01-27T21:35:51Z

crates/pg_workspace/src/workspace/server/migration.rs

+    use tempfile::TempDir;
+
+    fn setup() -> TempDir {
+        TempDir::new().expect("Failed to create temp dir")


tempfile is required to have proper test that work with canonicalize

psteinroe · 2025-01-27T21:36:15Z

pglsp.toml

+[migrations]
+migrations_dir = "migrations"
+after = 20230912094327


tested it locally but removed the migrations dir again

should we remove it? or comment out the properties?

good point :D

juleswritescode

Very beautiful PR and great description! Love it 😍

juleswritescode · 2025-01-29T07:49:24Z

pglsp.toml

+[migrations]
+migrations_dir = "migrations"
+after = 20230912094327


should we remove it? or comment out the properties?

juleswritescode · 2025-01-29T07:51:53Z

crates/pg_workspace/src/workspace/server/migration.rs

+    fn test_get_migration_non_migration_file() {
+        let migrations_dir = PathBuf::from("/tmp/migrations");
+        let path = migrations_dir.join("not_a_migration.sql");


I don't fully understand what we're checking here – are we checking what happens if the .sql file is empty? or if it doesn't match the pattern?

it doesnt match the pattern since it doesnt contain a timestamp

clarified it in the test name 👍

juleswritescode · 2025-01-29T07:54:23Z

crates/pg_workspace/src/workspace/server/migration.rs

+    use tempfile::TempDir;
+
+    fn setup() -> TempDir {
+        TempDir::new().expect("Failed to create temp dir")


juleswritescode · 2025-01-29T07:57:30Z

crates/pg_workspace/src/workspace/server/migration.rs

+        .and_then(|os_str| os_str.to_str())
+        .and_then(|file_name| {
+            let mut parts = file_name.splitn(2, '_');
+            let timestamp = parts.next()?.parse().ok()?;


I really liked the explanation in the PR description about how popular tools all use similar migrations formats. Should we add a comment somewhere here that we expect a <TimestampInMs>_<rest> format?

yes, added comments

juleswritescode · 2025-01-29T08:05:08Z

crates/pg_cli/src/commands/check.rs

@@ -56,7 +62,7 @@ impl CommandRunner for CheckCommandPayload {
    }

    fn should_write(&self) -> bool {
-        self.write || self.fix
+        false


sanity check: did you just change this for debugging?

no, I removed all the --fix stuff because ware not fixing anything. its a leftover from biomes check command.

juleswritescode · 2025-01-29T08:07:50Z

crates/pg_configuration/src/migrations.rs

+pub struct MigrationsConfiguration {
+    /// The directory where the migration files are stored
+    #[partial(bpaf(long("migrations-dir")))]
+    pub migrations_dir: String,


I don't know if we use bpaf-derive, but if we do, we could also parse into a PathBuf.

Examples of that were hard to find, so maybe that's not the recommended way – I'm not sure

that would be preferred, but biomes Merge does not work with PathBuf :(

juleswritescode · 2025-01-29T08:10:22Z

crates/pg_workspace/src/workspace/server.rs

    /// Check whether a file is ignored in the top-level config `files.ignore`/`files.include`
    fn is_ignored(&self, path: &Path) -> bool {
        let file_name = path.file_name().and_then(|s| s.to_str());
        // Never ignore PGLSP's config file regardless `include`/`ignore`
        (file_name != Some(ConfigName::pglsp_toml())) &&
            // Apply top-level `include`/`ignore
-            (self.is_ignored_by_top_level_config(path))
+            (self.is_ignored_by_top_level_config(path) || self.is_ignored_by_migration_config(path))


very sweet!

juleswritescode · 2025-01-29T08:13:24Z

crates/pg_workspace/src/workspace/server.rs

+
+                Some(&migration.timestamp <= ignore_before)
+            })
+            .unwrap_or(false)


What happens if we pass a migrations_dir but no after? Wouldn't the and_then return None, so the function would always return false?

yes, and we do not want to ignore any migration file if no after is specified do we?

psteinroe added 4 commits January 21, 2025 15:30

initial commit

a5bea50

save

3873318

progress

825a3ad

finish

8a215d3

psteinroe marked this pull request as ready for review January 27, 2025 21:29

psteinroe requested a review from juleswritescode January 27, 2025 21:33

psteinroe commented Jan 27, 2025

View reviewed changes

psteinroe added 2 commits January 27, 2025 20:34

cleanup

61a4478

cleanup

661efe9

psteinroe commented Jan 27, 2025

View reviewed changes

juleswritescode approved these changes Jan 29, 2025

View reviewed changes

pr review

0942a35

psteinroe mentioned this pull request Jan 29, 2025

lexer: No token found at position #184

Closed

psteinroe merged commit 3f8b479 into main Jan 29, 2025
5 checks passed

cli ergonomics #172

cli ergonomics #172

Uh oh!

Conversation

psteinroe commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juleswritescode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

psteinroe commented Jan 21, 2025 •

edited

Loading