Simplify Makefile for api_docgen

[Octachron]

This PR removes two undefined variables and the use of | in api_docgen/Makefile,
cc @shindere .

[shindere]

For the two commits: you could capitalize the first letter of the first line. It's kind of a title after all. You could also start that title with something describing what it affects. You kind of did in the first commit but "makefiles" suggests several makefiles are iinvolved, which is actually not the case, so you could replace this "makefiles" by the path of the file this commit deals with, as you did in the second commit, except that if you had put the name of the file at the beginning, you would have saved the "in" in the title. The first commit looks okay to me, just one suggestion: ``` #capitalize first letter ``` would be better as ``` # Capitalize first letter ``` (add a space and capitalize the first letter, precisely) You could also make the documentation of this funciton a bit more precise by explaining the first letter of what exactly is capitalized. Like if a string of several words is given as argument, is it only the first character of the first word which gets capitalized, or the first character of each word in the string? Regarding the second commit: Perhaps you could put spaces aroudn = signs when defining variables? Why do you write `$(DIRS:%=%):%:` rather than simply `$(DIRS):` ? Also, why did you remove the `|` sign in the targets? It would seem more correct to me to keep it, actually, because when you depend on ea directory, you oonly depend on its existence, not on whether its creation date is newer than the one of the target. Regarding rules like ``` build/compilerlibref/%.odoc:%.mld build/compilerlibref ``` I personally find them already rather difficult to read. Perhaps you could add a space after the columns there?

[dra27]

I think you're using filenames with colons in them? That's a Windows no-no, I'm afraid.

[Octachron]

@dra27 : there are no colon in filenames here? All filenames are in the [a..Z_]+ subset.

@shindere: I have updated the commit messages to signal that they concern api_docgen/Makefile.
Similarly, I have added some documentation for the up function (that only exists to please check_typo).
The order prerequisites have been restored.
And there are more white spaces.

[shindere]

Many thanks for taking the comments into account so promptly, @Octachron! Florian Angeletti (2021/02/03 01:48 -0800):

@dra27 : there are no colon in filenames here? All filenames are in the [a..Z_]+ subset.

It's rather hard to read that makefile, in my opinion. Perhaps you could include some general documentation about what's going on? Not mandatory at all, and it does not need to happen in this PR.

@shindere: I have updated the commit messages to signal that they concern api_docgen/Makefile.

Thanks, and sorry: I was wrong about the first commit which does actually concern two files. Perhaps the message could start with something like `api_docgen makefiles:` ?

Similarly, I have added some documentation for the `up` function (that only exists to please check_typo).

Oh! Could you explain a bit more, please? what is it that check-typo wants? For consistency, you could also document capitalize_one...

The order prerequisites have been restored. And there are more white spaces.

Regarding the change in `.gitignore`: you are removing `/api_docgen/html` and adding `/api_docgen/texi`. Can you confirm that this is what you want to do? I still do not understand the `$(DIRS):%:` line. Why isn't it simply `$(DIRS):` ? If it really needs to remain as it is, please at least add a white-space between the colon and the percent sign. Another thing that you may or may not want to address, here or elsewhere: I see things like `-nostdlib -hide Stdlib -lib Stdlib -t "OCaml library"` appearing several times. My intuition is that there is still room for some factorization here. One last thing: ``` echo $(stdlib_INPUT)> $@ ``` Here, too, a whitespace before the `>` would increase readability, in my opinion and there may be other occurrences, I didn't go through the patch line by line yet.

[Octachron]

It's rather hard to read that makefile, in my opinion.

Note that this Makefile is partially rewritten in the odoc PR (#9997).
If you wish for more documentation and refactorization, I would rather do it in this PR.

I still do not understand the $(DIRS):%: line. Why isn't it simply
$(DIRS):

Merely, the habit of using static patterns. You are right that it can be simplified further here.

Concerning check-typo, inlining up in capitalize_one makes the line goes over the check-typo limit, and I don't know how to split the line without making \n significant. (before this PR, the code was using $\n as a trick)

The .gitignore changes are correct (there was a duplicated api_docgen/html line above).

[shindere]

Florian Angeletti (2021/02/03 02:45 -0800):

Note that this Makefile is partially rewritten in the odoc PR (#9997).

OK, thanks!

If you wish for more documentation and refactorization, I would rather do it in this PR.

Why not, although the two things look a bit orthogonal to me.

> I still do not understand the `$(DIRS):%:` line. Why isn't it simply `$(DIRS):` Merely, the habit of using static patterns. You are right that it can be simplified further here.

OK.

Concerning `check-typo`, inlining `up` in `capitalize_one` makes the line goes over the check-typo limit, and I don't know how to split the line without making `\n` significant. (before this PR, the code was using `$\n` as a trick)

Ah, got it. I thought you were sayig that the whole thing was done to please check-typo regarding the generated files. It's not bad to have separated the two, I think. For the long lines, I think if you end a line with a \ you can have logical lines that spend over several physical lines.

The `.gitignore` changes are correct (there was a duplicated `api_docgen/html` line above).

OK.

[Octachron]

@shindere , do you approve the current state?

[shindere]

Yes, I just sent a nitpick commentyesterday about the commit messages but it seems it didn't pass through. Roughly, I was sayingthat the first message is missing a verb like fix, andthat it could be rephrased to say what it's really doing, since the warning is actually only the symptom. In the second commit message, I'd add a "the". Also, I was saying that "remove" seemsnot completely correct because it's not a simple removal, it required some rewriting for the thingtowork without the target, so I was suggesting "get rid of".