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

Warn on sqitch add file with double extension #724

Merged
merged 3 commits into from
Apr 8, 2023

Conversation

blairjordan
Copy link
Contributor

@blairjordan blairjordan commented Mar 16, 2023

  • Warn on sqitch add file with double extension: sqitch add includes extensions for each engine. Warn if user specifies a filename like myobjects.sql resulting in the myobjects.sql.sql

`sqitch add` includes extensions for each engine. Warn if user specifies an extension resulting in a filename like `myobjects.sql.sql`
@blairjordan blairjordan marked this pull request as ready for review March 17, 2023 01:49
Copy link
Collaborator

@theory theory left a comment

Choose a reason for hiding this comment

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

Handy! aCould you add a test for it in t/add.t?

lib/App/Sqitch/Command/add.pm Outdated Show resolved Hide resolved
@theory
Copy link
Collaborator

theory commented Apr 3, 2023

Try this:

--- a/lib/App/Sqitch/Command/add.pm
+++ b/lib/App/Sqitch/Command/add.pm
@@ -407,17 +407,17 @@ sub _add {
         file  => $file,
         error => $!
     );
-    
+
     # Warn if the file name has a double extension
     if ($file =~ m/\.(\w+)\.\1+$/) {
         my $ext = $1;
-        $self->warning(__x(
-            'Warning: file {file} has a double extension of {ext}',
+        $self->warn(__x(
+            'File {file} has a double extension of {ext}',
             file => $file,
             ext  => $ext,
         ));
     }
-    
+
     $self->info(__x 'Created {file}', file => $file);
 }
 
diff --git a/t/add.t b/t/add.t
index cd3e29a3..ea952b3d 100644
--- a/t/add.t
+++ b/t/add.t
@@ -3,7 +3,7 @@
 use strict;
 use warnings;
 use utf8;
-use Test::More tests => 238;
+use Test::More tests => 240;
 #use Test::More 'no_plan';
 use App::Sqitch;
 use App::Sqitch::Target;
@@ -414,14 +414,6 @@ EOF
         'Info should show $out created';
     unlink $out;
 
-    # # Test with file name having a double extension
-    $out = file 'test-add', 'duplicate_extension_test.sql.sql';
-    warning_is {
-        $add->_add('duplicate_extension_test.sql', $out, $tmpl, 'sqlite', 'add')
-    } [qr/double extension of sql/], 'Should get warning for file name with double extension';
-
-    unlink $out;
-    
     # Try with requires and conflicts.
     ok $add =  $CLASS->new(
         sqitch    => $sqitch,
@@ -448,6 +440,18 @@ BEGIN;
 COMMIT;
 EOF
     unlink $out;
+
+    # # Test with file name having a double extension
+    $out = file 'test-add', 'duplicate_extension_test.sql.sql';
+    $add->_add('duplicate_extension_test.sql', $out, $tmpl, 'sqlite', 'add');
+    is_deeply +MockOutput->get_info, [[__x 'Created {file}', file => $out ]],
+        'Info should show $out created';
+    is_deeply +MockOutput->get_warn, [[__x(
+        'File {file} has a double extension of {ext}',
+        file => $out,
+        ext => 'sql',
+    )]], 'Should have warned about double extension';
+    unlink $out;
 };
 
 # First, test  with Template::Tiny.

@blairjordan blairjordan force-pushed the patch-2 branch 2 times, most recently from 5c6474b to e4e690e Compare April 3, 2023 14:30
@blairjordan
Copy link
Contributor Author

Try this:

--- a/lib/App/Sqitch/Command/add.pm
+++ b/lib/App/Sqitch/Command/add.pm
@@ -407,17 +407,17 @@ sub _add {
         file  => $file,
         error => $!
     );
-    
+
     # Warn if the file name has a double extension
     if ($file =~ m/\.(\w+)\.\1+$/) {
         my $ext = $1;
-        $self->warning(__x(
-            'Warning: file {file} has a double extension of {ext}',
+        $self->warn(__x(
+            'File {file} has a double extension of {ext}',
             file => $file,
             ext  => $ext,
         ));
     }
-    
+
     $self->info(__x 'Created {file}', file => $file);
 }
 
diff --git a/t/add.t b/t/add.t
index cd3e29a3..ea952b3d 100644
--- a/t/add.t
+++ b/t/add.t
@@ -3,7 +3,7 @@
 use strict;
 use warnings;
 use utf8;
-use Test::More tests => 238;
+use Test::More tests => 240;
 #use Test::More 'no_plan';
 use App::Sqitch;
 use App::Sqitch::Target;
@@ -414,14 +414,6 @@ EOF
         'Info should show $out created';
     unlink $out;
 
-    # # Test with file name having a double extension
-    $out = file 'test-add', 'duplicate_extension_test.sql.sql';
-    warning_is {
-        $add->_add('duplicate_extension_test.sql', $out, $tmpl, 'sqlite', 'add')
-    } [qr/double extension of sql/], 'Should get warning for file name with double extension';
-
-    unlink $out;
-    
     # Try with requires and conflicts.
     ok $add =  $CLASS->new(
         sqitch    => $sqitch,
@@ -448,6 +440,18 @@ BEGIN;
 COMMIT;
 EOF
     unlink $out;
+
+    # # Test with file name having a double extension
+    $out = file 'test-add', 'duplicate_extension_test.sql.sql';
+    $add->_add('duplicate_extension_test.sql', $out, $tmpl, 'sqlite', 'add');
+    is_deeply +MockOutput->get_info, [[__x 'Created {file}', file => $out ]],
+        'Info should show $out created';
+    is_deeply +MockOutput->get_warn, [[__x(
+        'File {file} has a double extension of {ext}',
+        file => $out,
+        ext => 'sql',
+    )]], 'Should have warned about double extension';
+    unlink $out;
 };
 
 # First, test  with Template::Tiny.

@theory That's done the trick, thanks!

@blairjordan blairjordan requested a review from theory April 3, 2023 14:34
@theory theory merged commit 6fe849d into sqitchers:develop Apr 8, 2023
@theory
Copy link
Collaborator

theory commented Apr 8, 2023

Thank you!

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.

None yet

2 participants