tl;dr

We should start using only the hyphenated "auto-save" and its variants--auto_save, and AutoSave--and make any adjustments to existing code that seem practical and beneficial.

Overview

We're currently inconsistent in our spelling of autosave/auto-save throughout the codebase. Most of the time it's just a minor annoyance, but it can make text search harder and even result in inconsistent APIs and endpoints. It violates one of the best-named software rules of all time: The Principle of Least Surprise. 😛 Here's a quick breakdown of the situation:

Routes

Four of each:

Filenames

Non-test files:

$ for i in autosave auto-save auto_save AutoSave; do echo "$i: $(find . -name \*"$i"\* -not -path \*test\* | wc -l)"; done

autosave:   0
auto-save:  0
auto_save:  1
AutoSave:   7

Specifically...

$ for i in autosave auto-save auto_save AutoSave; do echo "$i:"; find . -name \*"$i"\* -not -path \*test\*; echo; done

autosave:

auto-save:

auto_save:
./experience_builder.auto_save.inc

AutoSave:
./src/AutoSaveData.php
./src/Controller/ApiAutoSaveController.php
./src/Controller/ApiConfigAutoSaveControllers.php
./src/AutoSave
./src/AutoSave/AutoSaveManager.php
./src/AutoSave/AutoSaveTempStore.php
./src/AutoSave/AutoSaveTempStoreFactory.php

Total code occurrences

Including code comments and variable names--which are kind of all over the place--in non-test files:

$ for i in autosave auto-save auto_save AutoSave; do echo "$i: $(find . -name '*'"$i"'*' -not -name Test.php | wc -l)"; done    

autosave:    1
auto-save:   0
auto_save:   1
AutoSave:   13

Other

  • At least a few JsonResponse-es have autosave in a key.
  • At least one JsonResponse has "auto-save" in a message string.
  • The "Component" field for Drupal.org issues is "Auto-save".

Proposed resolution

My conclusion: The precedent strongly favors two words, either separated with a hypen or underscore or camel-cased. Unfortunately, there are four API-impacting instances--four routes and a few API data keys that include the one-word form. Unless-and-until we start versioning the API, we'll probably just have to live with those. Other than that, I recommend...

  1. Standardize on the hyphenated "auto-save" and its variants--auto_save, and AutoSave.
  2. Document the decision. - Where?
  3. Decide if we need to make any changes to existing code/documentation. - Just change the term wherever it's found.
  4. Determine if there are any UI strings that we want to change. - There don't seem to be.
  5. Let the dev team know about it once it's been committed to avoid confusion.

User interface changes

None.

CommentFileSizeAuthor
xb-routes.png50.54 KBtraviscarden
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

traviscarden created an issue. See original summary.

wim leers’s picture

Component: Page builder » Auto-save
Assigned: Unassigned » traviscarden
Issue tags: +terminology

One more thing: the issue queue component is also hyphenated, because that seemed to match the dominant spelling.

We can change the APIs (PHP API, internal HTTP API) easily at this alpha stage.

Let's make it consistent, and execute your conclusion 🙏 So that means:

  1. 👍 Standardize on the hyphenated "auto-save" and its variants--auto_save, and AutoSave.
  2. 👍 Document the decision.
  3. Decide if we need to make any changes to existing code/documentation. Yes, let's make this consistent too.
  4. 👍 Determine if there are any UI strings that we want to change.
  5. Let the dev team know about it. No need, we can just do it.

Ideally, we'd be able to configure cspell to forbid autosave and Autosave, but allow auto-save, Auto-save and Auto-Save. That'd catch everything AFAICT?

traviscarden’s picture

Issue summary: View changes

I think I got all but one thing, @wim leers: Where should we document the standard? I didn't see a place that it seemed like it fit.

I've updated the issue summary to clarify that I mean to let the dev team know about it after it's been committed, just to prevent surprise and confusion.

Also, I want credit for the fact that my initial analysis in the issue summary already included the issue queue component. I don't want that counting against me if you and I ever get into some kind of contest, @wim leers. 😉

Finally, I see the cpsell job failed. Am I crazy, or did it fail because it found a forbidden word in the directive with which I forbade it in dictionary.txt? 🤨 Well, maybe it's an easy fix--maybe the inline comments confuse it... but it's time for me to log off for the night. @wim leers, maybe you know the solution off the top of your head.

fjgarlin’s picture

You might need to actually ignore the project's dictionary.txt from the cspell configuration.

fjgarlin’s picture

Cspell job fixed after the last commit.

traviscarden’s picture

Assigned: traviscarden » wim leers
Priority: Minor » Normal
Status: Active » Needs review

I guess that makes sense, @fjgarlin, since these are the first negative terms--in other words, dictionary.txt was probably already being scanned, but it always passed because it contained all of its own terms. 😛

So the tests pass now. Bumping priority since this affects APIs, and we don't want to start to drift and get out of sync. On that account, I'm going to go ahead and assign directly to @wim leers to see if we can rush it through. Ready for review!

wim leers’s picture

Title: Decide between autosave and auto-save and standardize » Standardize on `auto-save`, remove `autosave`
Assigned: wim leers » Unassigned
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community

#4: I suggested the dictionary as the way to both enforce and document it.

Thanks for getting this done so quickly!

wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, fellow nitpicker! 🤓😄

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.