Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve poor filename filtering feedback to user #252

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lw2011
Copy link

@lw2011 lw2011 commented Oct 10, 2022

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

@lw2011 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
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied.

@@ -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")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, applied.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied.

.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 {}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right, I changed the message.

@lw2011 lw2011 force-pushed the improve_bad_filename_feedback_to_user branch from f48a87c to 551d207 Compare November 29, 2022 15:23
@lw2011 lw2011 requested a review from jxs November 29, 2022 15:29
Lukasz Wisniowski and others added 5 commits November 29, 2022 16:32
* 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
@lw2011 lw2011 force-pushed the improve_bad_filename_feedback_to_user branch from 551d207 to e017426 Compare November 29, 2022 15:33
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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))$")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+ ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants