error handling when path to dir provided instead of file #109796
Conversation
|
Need to create a test to be run in the device lab following instructions found here: https://github.com/flutter/flutter/blob/master/dev/devicelab/README.md |
merging version check
…eliasyishak/flutter into windows-version-check-in-doctor
merging for latest code
- case 1: successful check (version 10 or greater) - case 2: `systeminfo` process call fails - case 3: version identified is less than 10
|
I don't think this is the right approach: in general this feels far too restrictive, especially as it is perfectly reasonable for folks to give screenshots whatever extension they want or use non-ascii characters. Is there an exception thrown if we create a file with an unsupported name, like "."? If so, we could catch that exception instead. But consider shifting from an allow based approach to a deny based approach |
|
Yeah it definitely was a more restrictive approach, and that's understandable if we would like to make it less restrictive. We can remove any character restriction and file type restriction on the pattern and just make sure the directory is valid? Because the tool does actually allow for files that don't even have any extensions, ie. |
isn't it reading the extension and then determining what encoding to use based on that? so if you pass |
|
As far as I can tell, the tool does not use the file extension to determine what kind of file output. |
this is simctl |
|
I'm not sure how simctl handles it but I have seen that the file gets generated, although I wasn't able to open it on my mac. Which was why I initially thought to add the extension requirement and possibly expand to different file types as needed. |
|
but simctl isn't the only backend here, and based on the snapshot type argument we can also create a "skia" and "rasterizer" type |
- added additional transformation for outputFile incase users pass a trailing directory delimiter (ie. '-o image.png/' will result in a file being output at $PWD/image.png
| /// return a 0 exit code even if outputFile is not valid | ||
| static File validateOutputFile(File outputFile, FileSystem fs) { | ||
|
|
||
| // Will make sure that if the user passes '-o image.png/' the file |
There was a problem hiding this comment.
@jonahwilliams i added this additional functionality to catch any errors if the user provides a trailing delimiter. Do you think it's worth keeping this or should the tool throw an exception?
There was a problem hiding this comment.
I would not modify the file the user provides to this script, either accept it if it is valid or deny if it isn't. Consider the case where someone uses this command in part of an automated test script, and said test script has a bug that gets an invalid path. You would want that to fail as soon as possible, instead of accidentally passing for some time, or failing because a different command that accepts a file didn't make the same change to the file name to make it valid
There was a problem hiding this comment.
Great point, I'll make the adjustments
| const Platform platform = LocalPlatform(); | ||
|
|
||
| // Conditional to check for trailing path separator | ||
| if (outputFile.path.endsWith(platform.pathSeparator)) { |
There was a problem hiding this comment.
This is available on path.
| 'The provided path cannot end with a path separator\n' | ||
| 'Path provided: "${outputFile.path}"' |
There was a problem hiding this comment.
I would rephrase this to indicate that we expect a file path but got a directory path.
| if (!fs.directory(outputFile.dirname).existsSync()) { | ||
| throwToolExit( | ||
| 'The provided path to file needs to have a directory that exists\n' | ||
| 'Directory: "${outputFile.dirname}" does not exist' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Use fs.file(outputFile).parent.existsSync() - I'm not sure if dirname includes the contents of absolute paths.
| } | ||
|
|
||
| // Conditional for validating filename | ||
| if (outputFile.basename.isEmpty || outputFile.basename == '.') { |
There was a problem hiding this comment.
what about .. or ../?
| }); | ||
| }); | ||
|
|
||
| group('Screenshot file validation', () { |
There was a problem hiding this comment.
Since all this command does now is check if the file exists, you only need a test case for 1) file exists and 2) file doesn't exist
Fixes: #109450
///).