ietoolkit icon indicating copy to clipboard operation
ietoolkit copied to clipboard

Create the iesave command

Open kbjarkefur opened this issue 2 years ago • 1 comments

kbjarkefur avatar Aug 23 '21 19:08 kbjarkefur

@kbjarkefur @Avnish95 , is this ready for review?

luizaandrade avatar Jan 30 '22 20:01 luizaandrade

  • [ ] we should standardize the idvar() option across all commands that use it -- it's IDvars() in iesave, idvar() in iecorrect (check others)
  • [x] change option dtaversion() to version()
  • [ ] if using is not specified, the error message is varlist not allowed. I assume this is a standard Stata syntax check. Is that the case? If it is not, maybe we can have a more information error message? Like "this commands requires that using be specified"?
  • [ ] (super minor) I think the quotation marks around the variable labels look a bit weird. They don't seem strictly required, since they are not present in the categories count. But agian, this is very minor.
  • [x] I don't see why we are using replace inside reportpath. I think the metadata should be replaced whenever the data is replaced, and that is the behavior when the file path is the same as the data's, right?
  • [x] I think the options would be more intuitive if all the report options were stored inside an option called report. Right now, you can specify report options such as noalpha or csv without getting an error message. I suggest we change the syntax to
 iesave using "/path/to/data.dta", ///
    idvars(varlist) dtaversion(version_number) ///
    [replace userinfo report([csv] [path("/path/to/report.md"] [noalpha])]

luizaandrade avatar Nov 08 '22 17:11 luizaandrade

we should standardize the idvar() option across all commands that use it -- it's IDvars() in iesave, idvar() in iecorrect (check others)

It is only these commands that has an option with these names. (ieduplicates use ieduplicates varname using ). Since both iesave and iecorrect allows for more than one ID variable, then I think it should be in plural. Changing idvar() to idvars() in iecorrect would be a breaking change but not if switched to IDvars(). If you agree, lets open up an issue for that, or lets fix immediately, but for iesave in this PR I think we should keep as is.

kbjarkefur avatar Nov 10 '22 10:11 kbjarkefur

change option dtaversion() to version() done in 7d43c54b3e6ffdf00f6ef3fb2c7fbb0846f03fa3

kbjarkefur avatar Nov 10 '22 10:11 kbjarkefur

if using is not specified, the error message is varlist not allowed. I assume this is a standard Stata syntax check. Is that the case? If it is not, maybe we can have a more information error message? Like "this commands requires that using be specified"?

What syntax do you use to get this error message? Seems like you also removed the , before the options when removing using path. Then I get the same error message as you do. If you run this do you get a different error than I do?

. sysuse auto, clear
(1978 automobile data)

. iesave, idvars(make) version(13.1)
using required
r(100);

kbjarkefur avatar Nov 10 '22 10:11 kbjarkefur

(super minor) I think the quotation marks around the variable labels look a bit weird. They don't seem strictly required, since they are not present in the categories count. But again, this is very minor.

This is to prevent errors in columns in case labels include , or |. Or to show if label contains leading or trailing spaces. I think this should be kept as this is not for publication and then I think the benefit of explicitly showing white space is worth the aesthetical trade off

kbjarkefur avatar Nov 10 '22 10:11 kbjarkefur

  • I don't see why we are using replace inside reportpath. I think the metadata should be replaced whenever the data is replaced, and that is the behavior when the file path is the same as the data's, right?
  • I think the options would be more intuitive if all the report options were stored inside an option called report. Right now, you can specify report options such as noalpha or csv without getting an error message. I suggest we change the syntax to

This is fixed in 4a9c54f17be0f1ed7b46cba14d9accf8677afc72. Helpfile not yet updates as I want to get first feedback from you @luizaandrade.

Report options not work like this iesave using "/dtapath/dta.dta", idvars(idvar) version(17) report and iesave using "/dtapath/dta.dta", idvars(idvar) version(17) report() creates a report with all default options. Default options meaning:

  • Use same path as data file but replace file extension: "/dtapath/dta.md"
  • Use .md format for report
  • Replace report file if replace is used for data file
  • Sort variables alphabetically

Then it is also possible to change these default behaviors by specify the command like this iesave using "/dtapath/dta.dta", idvars(idvar) version(17) report(path("/dtapath/dta.md") csv replace noalpha) where:

  • report(path("/dtapath/dta.md")) use path "/dtapath/dta.md" instead of "/dtapath/dta.md"
  • report(csv) use .csv format for report - superseded if report(path()) is used which require the extension to be explicitly set.
  • report(replace) replace report file regardless of what data file replace says
  • report(noalpha) keeps var order from data set in report

Let me know if this make sense to you and I will update help file

kbjarkefur avatar Nov 10 '22 14:11 kbjarkefur

if using is not specified, the error message is varlist not allowed. I assume this is a standard Stata syntax check. Is that the case? If it is not, maybe we can have a more information error message? Like "this commands requires that using be specified"?

What syntax do you use to get this error message? Seems like you also removed the , before the options when removing using path. Then I get the same error message as you do. If you run this do you get a different error than I do?

. sysuse auto, clear
(1978 automobile data)

. iesave, idvars(make) version(13.1)
using required
r(100);

Yes, when I enter it like this, this is the error I get. But I was typing like would for save:

.  iesave "run/output/iesave/auto.dta", id(make)
varlist not allowed
r(101);

luizaandrade avatar Nov 14 '22 15:11 luizaandrade

Good to go 🚀

luizaandrade avatar Nov 15 '22 17:11 luizaandrade