Regularize cmd/page function naming
(1) By anonymous on 2020-03-04 02:10:02
I was browsing the Fossil source code to figure out which function corresponds to which Fossil command or UI page. Mostly it's nicely labeled with COMMAND/WEBPAGE tag in preceding comments. I guess, that's what the build process is parsing to bind commands to functions.
But I noticed two intermixed conventions for the underlying function names:
1. `<name>_cmd()` for commands, `name_page()` for UI pages
egrep "void [^_]*_(cmd|page)" src/*.c
2. `cmd_<name>()` for commands, `page_name()` for UI pages
egrep "void (cmd|page)_" src/*.c
There're more entries in the first group, I'd guess it's an original intent, though I'm not sure if that's the case.
Maybe we should regularize the naming for these top-level functions, so the names follow the same convention? It's sort of Fossil API.
Is any of the conventions above more convenient/descriptive, that is preferrable?
(2) By anonymous on 2020-04-06 00:09:58 in reply to 1 [link]
Matched the COMMAND and WEBPAGE names to respective `_cmd` and `_page` functions [\[06afb7022\]](https://fossil-scm.org/fossil/info/06afb7022f6f4553), branch: api-cleanup.<br />Changes are to the function names, not the the commands. This follows the `_cmd/page` __suffix__ convention for the API function naming, with underscores used instead of punctuation marks allowed in COMMAND and WEBPAGE names. Now for the most part the names match; however, a few are left differing (see the list below). I'm not sure if we should match those too for the purposes of making the API convention somewhat uniform. ``` ------------------- COMMAND | FUNCTION_cmd -----------------------------|------------------------------ test-compress-2 | test_compress2 test-move-repository | test_move_repo new | create_repository git | gitmirror server | webserver 3-way-merge | delta_3waymerge rebuild | rebuild_database rss | timeline_rss smtpd | smtp_server winsrv | win32_service WEBPAGE | FUNCTION_page -----------------------------|------------------------------ favicon.ico | favicon forume1 | forum_main raw | rawartifact secureraw | secure_rawartifact juvlist | uvlist_json ------------------- ```
(3) By Florian Balmer (florian.balmer) on 2020-04-06 07:03:44 in reply to 2 [link]
I don't see the benefit, here. Such large-scale cosmetic changes are hard to test (i.e. did you try every single command?) and make diffs between releases much more complicated than they need to be (i.e. "diff-pollution" absorbing attention away from more critical parts).
(4) By anonymous on 2020-04-06 17:25:04 in reply to 3 [link]
> ...Such large-scale cosmetic changes are hard to test (i.e. did you try every single command?)
The changes are mostly concerned with the top-level API functions. These functions are called from `main()` by pointer, the bulk of the Fossil code does not care about these names by design.
So the impact of these changes is not a large-scale one, and indeed it's not hard to test. Here's what I did for tests:
1. `'make'` -- builds without errors
2. `'tclsh ./test/tester.tcl ./fossil -prot'` -- Fossil's test suite reports the same test statuses as for the base trunk revision
3. `'0:0'` - eye-balled over every line of the changes made, especially those that are not in the function's signature or comments:
fossil diff -w --from 5e28febf --to api-cleanup | grep "^[-+][^*v][^-+]"
4. Exercised the commands/pages that have the actual code changes (above)
This is a simple refactoring and is a great chance to benefit from the use of the expansive Fossil test suite.
As for the benefit here: the ["traditional"](https://fossil-scm.org/fossil/doc/trunk/www/adding_code.wiki) naming convention is reasonable; apart from readability, it helps spot instances of one command invoking another. Naming consistency helps new developers navigate the internals of the Fossil code or an occasional back-trace.
(5) By Daniel Dumitriu (danield) on 2021-03-17 19:42:18 in reply to 4 [link]
Last night I was reading "Adding Code", noticed the inconsistency and vaguely remembered this discussion. I agree with Arthur here. There is value in following a good convention and it helps having a well-organized codebase. And if in a commit we do only refactoring and clearly state this, there's not so much diff pollution left. Since he already did a large part of the job, I think we should consider merging this branch after bringing it to date.
(6) By Stephan Beal (stephan) on 2021-03-18 03:14:57 in reply to 5 [link]
> I agree with Arthur here. There is value in following a good convention and it helps having a well-organized codebase. The counter-argument is that the command- and web-page functions are not (with very few exceptions) called by developers. They're handled automatically by the build process and invoked only via the auto-generated command dispatcher. Developers don't search for those functions by name: they grep for "COMMAND: foo" or "WEBPAGE: foo" instead. i.e. it doesn't really matter what the functions are called. i wouldn't object to any such change, i just don't see any significant benefit in it because the functions in question aren't used by developers. But if it scratches your itch then, by all means, have at it.
(7) By jamsek on 2021-03-19 10:39:11 in reply to 6 [link]
I agree with both of you. If for no other reason than consistency, I agree with Daniel. But I can't argue with Stephan not seeing any significant benefit either. Although I think the cost is negligible considering most of the work has already been done by the looks of it, and there may be some future benefit beyond just good convention. In which case I, too, think it should be merged.