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

@@ -11,9 +11,9 @@ use crate::traits::DEFAULT_MIGRATION_TABLE_NAME;
 use crate::{AsyncMigrate, Error, Migrate};
 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

@@ -44,15 +40,12 @@ pub fn find_migration_files(
        .into_iter()
        .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

@@ -43,8 +43,8 @@ impl std::error::Error for Error {
 #[derive(Debug, TError)]
 pub enum Kind {
    /// 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

@@ -11,9 +11,10 @@ use crate::traits::DEFAULT_MIGRATION_TABLE_NAME;
 use crate::{AsyncMigrate, Error, Migrate};
 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

 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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve poor filename filtering feedback to user #252

Improve poor filename filtering feedback to user #252

lw2011 commented Oct 10, 2022 •

edited

jxs left a comment

jxs Nov 24, 2022

lw2011 Nov 29, 2022

jxs Nov 24, 2022

lw2011 Nov 29, 2022

jxs Nov 24, 2022

lw2011 Nov 29, 2022

jxs Nov 24, 2022

lw2011 Nov 29, 2022

jxs Nov 24, 2022

lw2011 Nov 29, 2022

jxs left a comment

jxs Dec 10, 2022

lw2011 Jan 12, 2023

	// 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

Are you sure you want to change the base?

Improve poor filename filtering feedback to user #252

Conversation

lw2011 commented Oct 10, 2022 • edited

jxs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lw2011 commented Oct 10, 2022 •

edited