[PATCH] D108765: [docs] Fix documentation of clang-format BasedOnStyle type

[Ludovic Jozeau via Phabricator via cfe-commits]

FederAndInk added a comment.

In D108765#2974153 <https://reviews.llvm.org/D108765#2974153>, @MyDeveloperDay wrote:

>   error: pathspec './plurals.txt' did not match any file(s) known to git
>   Traceback (most recent call last):
>     File "./dump_format_style.py", line 18, in <module>
>       subprocess.check_call(['git', 'checkout', '--', PLURAL_FILE])
>     File "/usr/lib/python3.8/subprocess.py", line 364, in check_call
>       raise CalledProcessError(retcode, cmd)
>   subprocess.CalledProcessError: Command '['git', 'checkout', '--', './plurals.txt']' returned non-zero exit status 1.

This is expected if `plurals.txt` is not in git yet, there is `call` that will do nothing on errors, but I use `check_call` to report errors from `git checkout -- plurals.txt`. To test, you can either replace temporarily the `check_call` by a `call` in clang/docs/tools/dump_format_style.py:18 or get the commit/create a temporary commit with plurals.txt

Maybe to simplify the testing/review procedure, I'll change `check_call` by `call` and if it is accepted, change it back to the checked version.



================
Comment at: clang/docs/tools/dump_format_style.py:9
 import re
+import inspect
+import subprocess
----------------
MyDeveloperDay wrote:
> FederAndInk wrote:
> > HazardyKnusperkeks wrote:
> > > I think these should be sorted.
> > ok, it will be done
> are these standard imports or are we going to have to pip install something?
These are all standard imports, no worries :) (https://docs.python.org/3/library/subprocess.html)

otherwise, I would have directly used python inflect/or any other package to solve the plural problem


================
Comment at: clang/docs/tools/dump_format_style.py:18
+PLURAL_FILE = os.path.join(os.path.dirname(__file__), 'plurals.txt')
+subprocess.check_call(['git', 'checkout', '--', PLURAL_FILE])
+plurals = set(open(PLURAL_FILE).read().splitlines())
----------------
MyDeveloperDay wrote:
> FederAndInk wrote:
> > HazardyKnusperkeks wrote:
> > > So you would add a plurals.txt in git and make the change visible through git diff? What about just reordering? I.e. `Strings` is on line 2, but after a change in line 1. Maybe sort the output?
> > > 
> > > I'm not against this procedure, but also not in favor. :)
> > This line is used to restore the version of plurals.txt to HEAD, so when calling the script multiple times, it keeps showing new plurals until plurals.txt gets committed.
> > 
> > > So you would add a plurals.txt in git and make the change visible through git diff?
> > 
> > yes, that's it
> > 
> > > What about just reordering?
> > 
> > I don't think we want ordering, it is ordered from first plural generated to last/new one, so git diff will only show new plurals
> I'm personally not in favour of this script calling back to git
Well, I was inspired by other python scripts in the llvm-project repo that use `subprocess` to call `git`, it just touches the plurals.txt files to allow the user calling the script multiple times and be warned of the new plurals each time until it is committed. We could do without it, but we would lose a part of automation and the user of the script would have to partly manage the plurals.txt file.

A semi-solution I see is to at least tell the user to use `git checkout -- plurals.txt` if they want to clean up plurals/regenerate them and see which ones are new and/or call `git diff -- plurals.txt` to show new plurals which may be less "problematic" than `git checkout -- plurals.txt`. What do you think?

Tl;dr of solutions:

1. reconsider as this technic is in use in other scripts in llvm-project
2. call `git diff` instead of `git checkout` leading to less consistent and precise messages
3. just tell the user what are their options (printing 'you can use `git checkout -- plurals.txt` or `git diff -- plurals.txt`'...)
4. other ideas?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108765/new/

https://reviews.llvm.org/D108765