Improve poor filename filtering feedback to user #252

lw2011 · 2022-10-10T16:14:33Z

Filtering filenames with regex has some drawbacks:

refinery_cli will silently skip filenames like "V1__first.base.rs" or "V2__second-base.rs" without any logging/feedback to the user
- user should be made aware what exactly is wrong with his filenames
refinery would allow filenames like "V1.5__first.base.rs" with decimal version numbers and will try to convert them into integers.
- refinery's database table stores versions as integers

jxs

Hi, and thanks for this! This is great work, only some minor nitpicks to address.

jxs · 2022-11-24T22:40:31Z

refinery_core/src/runner.rs

 use std::fmt::Formatter;

-// regex used to match file names
+// regex used to preliminary match migration semantics from filenames


Suggested change

// regex used to preliminary match migration semantics from filenames

// Regex used to preliminary match migration semantics from filenames.

jxs · 2022-11-24T22:47:14Z

refinery_core/src/util.rs

        .filter_map(Result::ok)
        .map(DirEntry::into_path)
-        // filter by migration file regex
+        // filter by migration type encoded in file extension


Suggested change

// filter by migration type encoded in file extension

// Filter by migration type encoded in file extension.

jxs · 2022-11-24T22:49:21Z

refinery_core/src/error.rs

    /// An Error from an invalid file name migration
-    #[error("migration name must be in the format V{{number}}__{{name}}")]
-    InvalidName,
+    #[error("migration filename must be in the format V{{number}}__{{name}}.rq|sql")]


Suggested change

#[error("migration filename must be in the format V{{number}}__{{name}}.rq|sql")]

#[error("migration filename must be in the format V{{number}}__{{name}}.rs|sql")]

Thanks, applied.

jxs · 2022-11-24T22:50:07Z

refinery_core/src/runner.rs

 use std::fmt::Formatter;

-// regex used to preliminary match migration semantics from filenames
+// regex matching migration semantics for filenames


Suggested change

// regex matching migration semantics for filenames

// Regex matching migration semantics for filenames.

jxs · 2022-11-24T22:54:38Z

refinery_cli/src/migrate.rs

-            .with_context(|| format!("could not read migration file name {}", path.display()))?;
+        let migration = Migration::unapplied(&filename, &sql).with_context(|| {
+            format!(
+                "Could not create new unapplied migration with filename {}",


do we want to mention to the user that we are creating an unapplied Migration? I'd say it can be a little confusing as it's giving internal information while the user should only care if the file could or could not be read wdyt?

Yes you are right, I changed the message.

* refinery_cli will silently skip filenames like "V1__first.base.rs" or "V2__second-base.rs" without any logging/feedback to the user * user should be made aware what exactly is wrong with his filenames * refinery will allow filenames like "V1.5__first.base.rs" with decimal version numbers and will try to convert them into integers. * refinery's database table stores versions as integers

* Feeds nice error messages to the user. * Before the change runner would not have a chance to inspect the formatting of input name.

* including file's extension

jxs

Sorry Łuksaz, just one more thing. Btw please don't force push as I lose context of which files I have already reviewed.

jxs · 2022-12-10T18:21:44Z

refinery_core/src/runner.rs

+// Regex matching migration semantics for filenames.
 pub fn file_match_re() -> Regex {
-    Regex::new(r"^([U|V])(\d+(?:\.\d+)?)__(\w+)").unwrap()
+    Regex::new(r"^(?P<type>[^_])(?P<version>[^_]+)__(?P<name>.+)(?P<extension>\.(sql|rs))$")


I'd rather here we still only capture U|V to not raise errors for other files in the path, btw why did you change the version from d+ ?

Hi @jxs ,

not raise errors for other files in the path

is it likely at all that someone uses filenames with two consecutive underscores in a project?

If the pattern would not search for __ , then I would leave V|U. Then the regex would need to accept both lower and upper cases letters. But it does match on __, so I find the checking of the version outside of regex better.

why did you change the version from d+

to output a better error to the user.

Additionally now fraction version are not allowed. For example, before the change, the filename V1.1__add.rs would have a match and the version would be 1.1 which is not supported by the database struct.

jxs · 2025-07-25T13:07:53Z

this is now stale, going to close, thanks Łukaz!

lw2011 force-pushed the improve_bad_filename_feedback_to_user branch 7 times, most recently from 2c9a8df to f48a87c Compare October 17, 2022 07:41

jxs requested changes Nov 24, 2022

View reviewed changes

lw2011 force-pushed the improve_bad_filename_feedback_to_user branch from f48a87c to 551d207 Compare November 29, 2022 15:23

lw2011 requested a review from jxs November 29, 2022 15:29

Lukasz Wisniowski and others added 5 commits November 29, 2022 16:32

core: inform user about formatting errors

13d40ed

* Feeds nice error messages to the user. * Before the change runner would not have a chance to inspect the formatting of input name.

macros: pass full filename to Migration::unapplied

a37cd7d

test: pass full filename in all tests

4a47ee2

* including file's extension

cli: use more specific error messages

e017426

lw2011 force-pushed the improve_bad_filename_feedback_to_user branch from 551d207 to e017426 Compare November 29, 2022 15:33

jxs reviewed Dec 10, 2022

View reviewed changes

jxs closed this Jul 25, 2025

	// regex used to preliminary match migration semantics from filenames
	// Regex used to preliminary match migration semantics from filenames.

	// filter by migration type encoded in file extension
	// Filter by migration type encoded in file extension.

	#[error("migration filename must be in the format V{{number}}__{{name}}.rq\|sql")]
	#[error("migration filename must be in the format V{{number}}__{{name}}.rs\|sql")]

	// regex matching migration semantics for filenames
	// Regex matching migration semantics for filenames.

Improve poor filename filtering feedback to user #252

Improve poor filename filtering feedback to user #252

Uh oh!

Conversation

lw2011 commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jxs 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!

jxs 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!

jxs commented Jul 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lw2011 commented Oct 10, 2022 •

edited

Loading