Issue 198: iefielkit aux functions other commands #201
Conversation
DenisseO
changed the title
luizaandrade
requested review from
kbjarkefur and
luizaandrade
and removed request for
kbjarkefur
|
@kbjarkefur @bbdaniels , can you take a look at these since you know the commands better? the idea is just to remove code that is repeated across commands |
| local file = substr("`using'", `r_lastslash' + 2, .) | ||
|
|
||
| * If a filename was specified, separate the file name from the file format | ||
| if "`file'" != "" { |
There was a problem hiding this comment.
When would this be empty? using/ is required and folders are defined as anything before a /. So if / exists then folders are anything before / and file is whatever is after and if there is no / then file is the same as using/? Right? Or is there any input for which this command stores somethin in folder but file is empty?
There was a problem hiding this comment.
Maybe if using/ is folderA/folderB/. But then the input is no longer a filename and it is a path. I think that that should either be an error or the command should be called ieaux_parse_filepath and then a path without a filename should be just as valid input as a filename with or without path.
There was a problem hiding this comment.
Yes, if there is no / then file is the same as using/
src/ado_files/iefieldkit_aux.do
Outdated
| cap program drop ieaux_folderpath | ||
| program ieaux_folderpath | ||
|
|
||
| syntax, [description(string) folderpath(string)] |
There was a problem hiding this comment.
Why are both these options optional? When does it make sense to run this command without any options specified?
There was a problem hiding this comment.
This command could have an option to recursively create a path that does not exist.
There was a problem hiding this comment.
I have updated this command to have as input ieaux_parse_filepath command
9cac6b2
src/ado_files/iefieldkit_aux.do
Outdated
| /******************************************************************************* | ||
| TEST IF THE FILE EXTENSION IS THE CORRECT | ||
| - option "fileext" is a namelist of the correct extensions that the file may only have | ||
| - option "testfileext" is the current file extension |
There was a problem hiding this comment.
What does "current" file extension mean? Based on my understanding when reading the code I think a better name for this option would be defaultextension() as it is applied as the default to the filename when no file extension is used
There was a problem hiding this comment.
Wait, I read this again. This is the extension that we should test? I think it is much better if we were to get that from using. So take the content of using/ and use the command ieaux_filename/ieaux_parse_filename/ieaux_parse_filepath (or whatever it will be called) to get the extension in using/ (if any) and then test it towards the elements in fileext(namelist). In addition to the main commands in iefieldkit benefitting from on these aux/utility commands, these aux/utility commands should benefit from each other.
There was a problem hiding this comment.
I totally agree with this.
I added these changes here:
506a71e
Co-authored-by: Kristoffer Bjärkefur <[email protected]>
Co-authored-by: Kristoffer Bjärkefur <[email protected]>
src/ado_files/iefieldkit_aux.do
Outdated
| syntax using/, testfileext(string) | ||
| ieaux_filename using `using' | ||
|
|
||
| if !missing("`r(file)'") { | ||
| * Check if the file extension is the correct | ||
| local ext "" | ||
| foreach value in `testfileext' { | ||
| if (".`value'" == "`r(fileext)'") local errorfile 1 | ||
| local ext `".`value' `ext'"' | ||
| } | ||
|
|
||
| local wcount = `: word count `testfileext'' | ||
| if ("`errorfile'" != "1") & ("`r(fileext)'" != "1") { | ||
| if `wcount' > 1 local pluralms= "s" | ||
| noi di as error `"{phang}The file {bf:`using'} may only have the extension format`pluralms' [`ext']. The format [`r(fileext)'] is not allowed.{p_end}"' | ||
| error 198 | ||
| } | ||
|
|
||
| * If no file extension was used, then add the extension | ||
| if "`r(fileext)'" == "1" { | ||
| local ext = word("`testfileext'",1) // If there are more than one extension, get first | ||
| local using "`using'.`ext'" | ||
| } | ||
| } | ||
| return local using `using' |
There was a problem hiding this comment.
| syntax using/, testfileext(string) | |
| ieaux_filename using `using' | |
| if !missing("`r(file)'") { | |
| * Check if the file extension is the correct | |
| local ext "" | |
| foreach value in `testfileext' { | |
| if (".`value'" == "`r(fileext)'") local errorfile 1 | |
| local ext `".`value' `ext'"' | |
| } | |
| local wcount = `: word count `testfileext'' | |
| if ("`errorfile'" != "1") & ("`r(fileext)'" != "1") { | |
| if `wcount' > 1 local pluralms= "s" | |
| noi di as error `"{phang}The file {bf:`using'} may only have the extension format`pluralms' [`ext']. The format [`r(fileext)'] is not allowed.{p_end}"' | |
| error 198 | |
| } | |
| * If no file extension was used, then add the extension | |
| if "`r(fileext)'" == "1" { | |
| local ext = word("`testfileext'",1) // If there are more than one extension, get first | |
| local using "`using'.`ext'" | |
| } | |
| } | |
| return local using `using' | |
| syntax using/, allowed_exts(string) [default_ext(string)] | |
| *Parse the unput | |
| ieaux_filename using `using' | |
| local this_file "`r(file)'" | |
| local this_ext = subinstr("`r(file)'",".","") | |
| *Test if using has no file | |
| if missing("`this_file'") { | |
| noi di as error `"{phang}The path {bf:`using'} does not have a file name for which extension can be tested.{p_end}"' | |
| error 198 | |
| } | |
| *Test is using has no file extension | |
| else if missing("`this_ext'") { | |
| *Test if no deafult ext was provided | |
| if missing("`default_ext'") { | |
| noi di as error `"{phang}The file in {bf:`using'} does not have a file extension and no default was provided.{p_end}"' | |
| error 198 | |
| } | |
| *Apply the deafult extension | |
| else { | |
| local return_file "`using'.`default_ext'" | |
| } | |
| } | |
| * Using has both file and extension | |
| else { | |
| * Test if extension is among the allowed extensions | |
| if `: list this_ext in allowed_exts' == 1 { | |
| *Extension is allowed, return file name as is | |
| local return_file "`using'" | |
| } | |
| * File extension used is not allowed | |
| else { | |
| noi di as error `"{phang}The file extension [`this_ext'] in file {bf:`using'} is not allowed. Allowed extensions: [`allowed_exts'].{p_end}"' | |
| error 198 | |
| } | |
| } | |
| *Return checked filename | |
| return local filename "`return_file'" |
I would approach this command like this
No description provided.