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

Consider better names for a few commands #36

Open
arthur-shaw opened this issue Feb 16, 2024 · 4 comments · Fixed by #42
Open

Consider better names for a few commands #36

arthur-shaw opened this issue Feb 16, 2024 · 4 comments · Fixed by #42
Labels
solved but not yet published ✅ This issue is resolved, but not published on SSC yet, nor merged to main

Comments

@arthur-shaw
Copy link
Contributor

Names that don't quite communicate intent/use

There are a few commands whose names don't come naturally.

The vars and vals refer to variable and value labels, respectively, but something in the name seems to suggest that the'll return variables and values, respectively.

  • lbl_list_matching_vars
  • lbl_list_matching_vals

It might be better to have:

  • lbl_list_matching_varlbls
  • lbl_list_matching_vallbls

Some assert commands seem one or two words short of adequately descriptive:

  • lbl_assert_varlbls might be clearer as lbl_assert_have_varlbls

varlbl and vallbl too close?

Additionally, I have mixed feelings about commands whose name differs by a single letter, for an abbreviation that may or may not be widely accepted by users.

First, for Stata users, are varlbl and vallbl readibly intelligble terms?
Second, are there other ways to represent these entities in writing that are succinct but not subject to errant keystrokes (e.g., if r becomes l, it's a different command; if vallbl drops on of the the two first l characters, it's no longer a command).

@kbjarkefur
Copy link
Collaborator

kbjarkefur commented Feb 19, 2024

I fully agree with varlbls > vars and vallbls > vals since it is more descriptive what it returns.

I both see your points about varlbl and vallbl at the same time that similarity is common in Stata. Compate lab val and lab var.

Would splitting up the words highlight the difference a little bit more?

  • lbl_list_matching_var_lbls
  • lbl_list_matching_val_lbls

@arthur-shaw
Copy link
Contributor Author

Would splitting up the words highlight the difference a little bit more?

  • lbl_list_matching_var_lbls
  • lbl_list_matching_val_lbls

Yes, I think that does the trick--at, least for my eyes.

For renaming functions, would we need to do anything more than find-replace-all for the whole project (assuming that operation also renames files)?

Here's the scope. I may have missed something.

  • lbl_list_matching_vars -> lbl_list_matching_var_lbls
  • lbl_list_matching_vals -> lbl_list_matching_val_lbls
  • lbl_list_long_varlbl -> lbl_list_long_var_lbl
  • lbl_list_no_varlbl -> lbl_list_no_var_lbl
  • lbl_assert_varlbls -> lbl_assert_have_var_lbls

@kbjarkefur
Copy link
Collaborator

I have been thinking on how adodown could do this. But I have only come with how adodown reliably updates filenames and pkg-content. Not as obvious how to programmatically update where that command is used in other ado-files or how to update it in the test-files (especially if used from other commands test files)

I think in the meanwhile, what you are suggesting is the best approach. Search the whole clone and replace them one by one. Should not be prohibitively labor intensive.

So this in the release-v1.0 branch or a branch off of it.

@arthur-shaw
Copy link
Contributor Author

I have been thinking on how adodown could do this. But I have only come with how adodown reliably updates filenames and pkg-content. Not as obvious how to programmatically update where that command is used in other ado-files or how to update it in the test-files (especially if used from other commands test files)

I think there's probably no easy way to do that, absent some find-replace that traverses all files in the project (e.g., ingest each file, find-replace old name with new, rewrite file).

The reasonable actions seem to be:

  • Check that the user is ready to perform this dangerous operation (e.g., has commited everthing and has a fallback if things don't go well)
  • Rename matching files in src/ado, src/mdhlp, and src/sthlp
  • Update entries in {pkgname}.pkg
  • Give the user a list of other actions to consider doing (e.g., find-replace in ado and md files, rebuild sthlp files, re-run tests)

@arthur-shaw arthur-shaw linked a pull request Feb 26, 2024 that will close this issue
@arthur-shaw arthur-shaw added the solved but not yet published ✅ This issue is resolved, but not published on SSC yet, nor merged to main label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solved but not yet published ✅ This issue is resolved, but not published on SSC yet, nor merged to main
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants