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

FederAndInk added a comment. Also, don't we want to change the title and summary of the commit/revision? Because it does not reflect the real changes now in the repo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/

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

FederAndInk added a comment. Thanks for the merge, I now understand more how this all works and what we want from these scripts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/ https://reviews.llvm.org/D108765

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG9e8fff26f374: [clang-format][docs] Fix documentation of clang-format BasedOnStyle type (authored by FederAndInk, committed by MyDeveloperDay). Changed prior to commit:

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

FederAndInk added a comment. @MyDeveloperDay is it all good? I'm sorry for all the misunderstanding, Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/ https://reviews.llvm.org/D108765 ___

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

FederAndInk updated this revision to Diff 370953. FederAndInk added a comment. only print info on generated plurals if there is a new plural Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/ https://reviews.llvm.org/D108765 Files:

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

MyDeveloperDay added a comment. I still think the printing is unnecessary, tell me when the plurals will be different otherwise keep quiet? otherwise, it confuses people making them think they have to do something Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

FederAndInk updated this revision to Diff 370944. FederAndInk added a comment. remove one print l17, tell me if we don't want the second print (now l17) fixed typo in the script coment -> comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

FederAndInk added inline comments. Comment at: clang/docs/tools/dump_format_style.py:17 +PLURALS_FILE = os.path.join(os.path.dirname(__file__), 'plurals.txt') +print(f'generated plurals (for yaml type) are stored in {PLURALS_FILE}') +print('you can use `git checkout --

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

MyDeveloperDay added inline comments. Comment at: clang/docs/tools/dump_format_style.py:17 +PLURALS_FILE = os.path.join(os.path.dirname(__file__), 'plurals.txt') +print(f'generated plurals (for yaml type) are stored in {PLURALS_FILE}') +print('you can use `git checkout --

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

FederAndInk updated this revision to Diff 370883. FederAndInk added a comment. rebase main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/ https://reviews.llvm.org/D108765 Files: clang/docs/ClangFormatStyleOptions.rst

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

MyDeveloperDay added a comment. Could you just rebase, I'm not getting a clean merge of ClangFormatStyleOptions.rst Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/ https://reviews.llvm.org/D108765

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

FederAndInk marked 8 inline comments as done. FederAndInk added a comment. Ok, so I removed git invocation and I tell the user what are their options, at least warnings are emitted for new plurals, and they will be shown in git status/diff and in revisions. Repository: rG LLVM Github

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

FederAndInk updated this revision to Diff 370796. FederAndInk added a comment. remove git invocation and user input, tell the user what the plurals are used for Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/

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

MyDeveloperDay added a comment. For me it feels enough to output to stderr that we are missing plurals and what they are for, it’s not like you can say what they should be is it? This will happen so infrequently that it’s not worth the environment issues that checking git will cause.

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

MyDeveloperDay requested changes to this revision. MyDeveloperDay added a comment. This revision now requires changes to proceed. I think interacting with git or even blocking for input doesn’t feel right I have an issue out on llvm preserve checks that would run this tool and that couldn’t

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

FederAndInk added inline comments. Comment at: clang/docs/tools/dump_format_style.py:21-25 + # To allow testing with an untracked PLURAL_FILE + open(PLURAL_FILE, 'w').close() # TODO: remove this line when review is accepted + # TODO: use check_call when review is accepted +

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

HazardyKnusperkeks accepted this revision. HazardyKnusperkeks added a comment. From my point we can try that one, if there are problems we have plenty of time to revert it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/

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

FederAndInk updated this revision to Diff 370751. FederAndInk added a comment. use correct python assignment from tuple, ask the user if they want to invoke git, use call instead of check_call to allow testing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

FederAndInk added a comment. In 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 >

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

MyDeveloperDay added a comment. 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 subprocess.check_call(['git', 'checkout', '--', PLURAL_FILE]) File

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

MyDeveloperDay added inline comments. Comment at: clang/docs/tools/dump_format_style.py:9 import re +import inspect +import subprocess FederAndInk wrote: > HazardyKnusperkeks wrote: > > I think these should be sorted. > ok, it will be done are these standard

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

FederAndInk updated this revision to Diff 369519. FederAndInk marked an inline comment as done. FederAndInk added a comment. add common plural rules, use python3 explicitly, reorder imports Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/

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

FederAndInk marked 2 inline comments as done. FederAndInk added inline comments. Comment at: clang/docs/tools/dump_format_style.py:9 import re +import inspect +import subprocess HazardyKnusperkeks wrote: > I think these should be sorted. ok, it will be done

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

FederAndInk marked an inline comment as done. FederAndInk added inline comments. Comment at: clang/docs/tools/dump_format_style.py:26 +def register_plural(singular: str, plural: str): + if plural not in plurals: HazardyKnusperkeks wrote: > FederAndInk wrote:

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

HazardyKnusperkeks added a comment. In D108765#2972159 , @FederAndInk wrote: > And again, I don't really understand if we are allowed or not to pull in a > dependency such as pluralizer or inflect, this would be another idea My Python knowledge is

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

FederAndInk added inline comments. Comment at: clang/docs/tools/dump_format_style.py:26 +def register_plural(singular: str, plural: str): + if plural not in plurals: MyDeveloperDay wrote: > This failed for me with invalid syntax Oh, ok, sorry, I might be

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

MyDeveloperDay added inline comments. Comment at: clang/docs/tools/dump_format_style.py:26 +def register_plural(singular: str, plural: str): + if plural not in plurals: This failed for me with invalid syntax Repository: rG LLVM Github Monorepo CHANGES

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

FederAndInk updated this revision to Diff 369431. FederAndInk added a comment. generate plurals, for now supporting -y ending (-y to -ies/-ys), track generated plurals and show new ones to be checked Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

FederAndInk added a comment. Ok, here is my proposal for plurals, I have some ideas, I think the safest/most complete would be to have a file tracking generated plurals and tell the user of the script to check newly generated plurals then add them to git, this would also allow reviewers to see

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

HazardyKnusperkeks added inline comments. Comment at: clang/docs/tools/dump_format_style.py:23 + plurals = { +'IncludeCategory': 'IncludeCategories' + } MyDeveloperDay wrote: > HazardyKnusperkeks wrote: > > FederAndInk wrote: > > > HazardyKnusperkeks

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

MyDeveloperDay added inline comments. Comment at: clang/docs/tools/dump_format_style.py:23 + plurals = { +'IncludeCategory': 'IncludeCategories' + } HazardyKnusperkeks wrote: > FederAndInk wrote: > > HazardyKnusperkeks wrote: > > > Could you not just check

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

HazardyKnusperkeks added inline comments. Comment at: clang/docs/tools/dump_format_style.py:23 + plurals = { +'IncludeCategory': 'IncludeCategories' + } FederAndInk wrote: > HazardyKnusperkeks wrote: > > Could you not just check if there is a y at the end

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

FederAndInk added inline comments. Comment at: clang/docs/tools/dump_format_style.py:23 + plurals = { +'IncludeCategory': 'IncludeCategories' + } HazardyKnusperkeks wrote: > Could you not just check if there is a y at the end and replace it with ies, >

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

HazardyKnusperkeks accepted this revision. HazardyKnusperkeks added a comment. This revision is now accepted and ready to land. Looks good and I agree with the choices. Comment at: clang/docs/tools/dump_format_style.py:23 + plurals = { +'IncludeCategory':

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

FederAndInk added a comment. > look at the html Done, thanks > I assume you don't have commit access, we'll need your name and email address Here it is: Ludovic Jozeau it should also be on my profile > but if you think you might like to work on some other things, it might be > worth get

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

MyDeveloperDay added a comment. In D108765#2969036 , @FederAndInk wrote: > For now, I handle plural manually, but it can be changed, I also kept > Unsigned, what are your thoughts? I think thats ok for now, did you try building the file with

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

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. LGTM, thanks for adding that and fixing the spelling mistake, let the others have time to chip in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108765/new/

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

FederAndInk added a comment. For now, I handle plural manually, but it can be changed, I also kept Unsigned, what are your thoughts? Thanks for being so kind and responsive, it's really great to work on that :) as it is my first contribution to clang. Repository: rG LLVM Github Monorepo

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

FederAndInk updated this revision to Diff 369081. FederAndInk added a comment. Use yaml type style for clang-format documentation (`String`, `Integer`, `List of Strings`, ...) instead of c++ types. Fix typo in clang/Format/Format.h Regenarate ClangFormatStyleOptions.rst Repository: rG LLVM

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

FederAndInk added a comment. Ok, well, the reason I proposed this patch in the first place was that I have been working on a `.clang-format` schema (https://json-schema.org/) :) and I spotted the inconsistency. I checked, and clang-format reports an error if we give a negative value to an

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

MyDeveloperDay added a comment. I'm good with words like `List of Strings` but I don't think we need `Enum` `unsigned` I think Integer, I'm not sure what the code is even going to do if you supply a -ve, Warn I hope! (there's contribution idea number 2 for you right there!) ;-) Repository:

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

FederAndInk added a comment. What about: `unsigned`? And enum or struct types such as `BracketAlignmentStyle`, `ArrayInitializerAlignmentStyle`, ...? Should we add something like: `Enum BracketAlignmentStyle` and `Dictionnary BraceWrapping`? The complete list: 53 bool

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

HazardyKnusperkeks requested changes to this revision. HazardyKnusperkeks added a comment. As the one who wrote that: 1. Yes that part is not auto generated, because it is not in the `format.h`. 2. I'm more in favor of removing the `std::` from the documentation. 3. It is for the YAML

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

MyDeveloperDay added subscribers: HazardyKnusperkeks, owenpan, krasimir, sammccall, curdeius, klimek. MyDeveloperDay added a comment. In D108765#2967363 , @FederAndInk wrote: > Thank you for your explanations, I understand now. > > But as I look into

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

FederAndInk added a comment. Thank you for your explanations, I understand now. But as I look into `clang/docs/tools/dump_format_style.py` I see that it does not entirely generate `clang/docs/ClangFormatStyleOptions.rst` it replaces the lines between `{START,END}_FORMAT_STYLE_OPTIONS` I

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

MyDeveloperDay requested changes to this revision. MyDeveloperDay added a comment. This revision now requires changes to proceed. Thank you for your submission, but... 1. This is not the way to change this file, its autogenerated from clang/include/Format/Format.h using

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

FederAndInk created this revision. FederAndInk requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fix little inconsistency and use `std::string` (which is used everywhere else) instead of `string` Repository: rG LLVM Github Monorepo