search

worldbank / iefieldkit

Stata commands designed for Impact Evaluations field work. These are tools that are used during/after a survey in the field for data quality monitoring.
MIT License
38 stars 18 forks source link

iecodebook: Check file existence #144

Closed bbdaniels closed 4 years ago

bbdaniels commented 4 years ago

What does 'other syntaxes' mean here? Other manipulation? Other operations? 'Syntax' is merely the way in which a sentence in a language is defined, so as it stands this is not clear. If the extension is left off, this creates an xls file which it will then happily overwrite. The error check for an existing file needs fixing.

Fix is needed on https://github.com/worldbank/iefieldkit/blob/5069c473cdf90289a594e56b0e0440690053176b/src/ado_files/iecodebook.ado#L31-L74

kbjarkefur commented 4 years ago

I have implemented the file extension test. .xlsx, .xls and no extension are the only allowed values. If no extension is provided then ".xlsx" is added to "`using'".

So far so good, but I see a problem in these lines: https://github.com/worldbank/iefieldkit/blob/5069c473cdf90289a594e56b0e0440690053176b/src/ado_files/iecodebook.ado#L248

and

https://github.com/worldbank/iefieldkit/blob/5069c473cdf90289a594e56b0e0440690053176b/src/ado_files/iecodebook.ado#L667

We need to update those two lines if we want to keep allowing .xls. @bbdaniels what do you think?

bbdaniels commented 4 years ago

is there any functional difference? I am happy to allow xls if it works fully the same. this can be handled by instead replacing .xls with _report.xls, as that will work whether or not there is an extra x at the end right? (I assume Microsoft did the naming that way because they are as lazy coders as I am and realized this would work)

as a side note that looks out of date -- line 667 above should be _report now. does issues by default point at the master branch?

bbdaniels commented 4 years ago

done in beaafa0daa683e14ed7598eeb4051cca52e12c20

kbjarkefur commented 4 years ago

The code snippets above points to a commit, but I seem to have found that line from the master copied the link from the master branch, so it points to the most recent commit there. My bad but you got it.

kbjarkefur commented 4 years ago

I think beaafa0 borders being a bit too hacky. It is not great from a maintainability point. But since this is not likely to have to change and it is not me being the main maintainer for this command, I am ok with solving it like this.

bbdaniels commented 4 years ago

Agreed and closed 😂