Use yaml types for clang-format documentation (String, Integer, List of Strings, ...) instead of c++ types
track generated plurals in a file
Fix typo in clang/Format/Format.h
Differential D108765
[docs][clang-format] use yaml types style FederAndInk on Aug 26 2021, 7:31 AM. Authored by
Details Use yaml types for clang-format documentation (String, Integer, List of Strings, ...) instead of c++ types track generated plurals in a file Fix typo in clang/Format/Format.h
Diff Detail
Event TimelineComment Actions Thank you for your submission, but...
I think the main problem is options like this: Here we say its a std::vector<RawStringFormat> which doesn't really tell you much, as its actually just a YAML array of RawStringFormat records without actually telling you what a RawStringFormat record contains RawStringFormats: - Language: TextProto Delimiters: - 'pb' - 'proto' EnclosingFunctions: - 'PARSE_TEXT_PROTO' BasedOnStyle: google - Language: Cpp Delimiters: - 'cc' - 'cpp' BasedOnStyle: llvm CanonicalDelimiter: 'cc' So whilst I see that you were being consistent I kind of feel its in the wrong direction, we should be moving away from using C++ names here but more explaining what the YAML should look like. Alas the C++ in Format.h is important because the types are needed because we are generating the documentation directly from the code. But in most of places we are using an enumeration like this one. we are not saying clang::FormatStyle::RefernceAlignmentStyle and I'm not convinced we'd want to I hope that helps, but thank you for the patch. Comment Actions 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 understand your point, but as of now, the inconsistency comes from the part that is not auto-generated, are you suggesting editing dump_format_style.py to have simpler types such as string? Then how should we replace std::vector? Something like Type[] e.g. string[]? Or maybe we should first include BasedOnStyle into dump_format_style.py. Then take care of how to render types? What do you suggest? I am genuinely asking, as I really don't know what would be the best way to do things. Maybe we should include other people? I don't really know who to add as reviewers for that, but I think, the way to show types, should be discussed? As for detailing RawStringFormat, it wasn't the purpose of this patch, and maybe it should have its own? Comment Actions You are correct the file isn't 100% generated and some of it comes from another .h file too. But now we have you interesting in making a contribution which you clearly are lets think about how we might do this. To hook into the clang-format team I always recommend adding the #clang-format project, (which I added to this review), but also I recommend passing the review via @krasimir , @HazardyKnusperkeks , @curdeius there are some others who are here often like @owenpan and @sammccall and of course @klimek (who started all this). Please also of course add me @MyDeveloperDay I try to check the reviews daily as one of my frustrations was not being able to get things reviewed so I try to be pretty active. From my perspective I do like the idea of substituting out the std::string and std::vector for something like string[] how about we start with something simple like trying to fix the cases for AttributeMacros (std::vector<std::string>) maybe with just simple substitution. We can pass that via the rest of the team and see what they feel. AttributeMacros (in configuration string[]) or something like that, I'm not convinced anyone is using this documentation to know its a std::vector! Comment Actions As the one who wrote that:
Comment Actions 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 -> Boolean 18 unsigned -> ? 9 std::vector<std::string> -> List of Strings 5 std::string -> String 4 AlignConsecutiveStyle -> ? 2 int -> Integer 1 UseTabStyle -> ? 1 TrailingCommaStyle -> ? 1 string -> String 1 std::vector<RawStringFormat> -> ? 1 std::vector<IncludeCategory> -> ?... 1 SpacesInLineComment 1 SpacesInAnglesStyle 1 SpaceBeforeParensOptions 1 SpaceAroundPointerQualifiersStyle 1 SortJavaStaticImportOptions 1 SortIncludesOptions 1 ShortLambdaStyle 1 ShortIfStyle 1 ShortFunctionStyle 1 ShortBlockStyle 1 ReturnTypeBreakingStyle 1 ReferenceAlignmentStyle 1 PPDirectiveIndentStyle 1 PointerAlignmentStyle 1 OperandAlignmentStyle 1 NamespaceIndentationKind 1 LanguageStandard 1 LanguageKind 1 LambdaBodyIndentationKind 1 JavaScriptQuoteStyle 1 IndentExternBlockStyle 1 IncludeBlocksStyle 1 EscapedNewlineAlignmentStyle 1 EmptyLineBeforeAccessModifierStyle 1 EmptyLineAfterAccessModifierStyle 1 DefinitionReturnTypeBreakingStyle 1 BreakTemplateDeclarationsStyle 1 BreakInheritanceListStyle 1 BreakConstructorInitializersStyle 1 BracketAlignmentStyle 1 BraceWrappingFlags 1 BraceBreakingStyle 1 BitFieldColonSpacingStyle 1 BinPackStyle 1 BinaryOperatorStyle 1 ArrayInitializerAlignmentStyle Comment Actions 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!) ;-) Comment Actions 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 option expecting an unsigned. In the schema I am able to specify a minimum and I think it's appropriate to give the information to the user that it expect a positive/unsigned integer, what do you think? Also, interesting question, how do we want to handle plural, as the formulation 'List of Types' introduces it. If we do it manually, it won't scale. We could include a dependency in python to something like inflect. I'll upload a new patch soon. Comment Actions 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 Comment Actions 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. Comment Actions LGTM, thanks for adding that and fixing the spelling mistake, let the others have time to chip in. Comment Actions I think thats ok for now, did you try building the file with sphinx-build, I run `/usr/bin/sphinx-build -n ./docs ./html` Then go and look at the html
That is how it should be right ;-). I assume you don't have commit access, we'll need your name and email address if you want one of us to land it, so we can accredit you with the contribution, but if you think you might like to work on some other things, it might be worth get ting the "getting commit access" process started. I think its always good to wait to give others some chance to comment before we commit, as we are all in different timezones. Comment Actions
Done, thanks
Here it is: Ludovic Jozeau <federandink@gmail.com> it should also be on my profile
Yeah, after reading https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access, I think I'll wait until I do another contribution. :)
Ok, that's fine. Comment Actions Looks good and I agree with the choices.
Comment Actions 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 new plurals generated. I'll put the updated revision of my idea soon. Another idea without tracking plurals would be to show the list of generated word -> plural but I think it would add noise over time and wouldn't add value, but if we don't want to add a plurals.txt file in git it would be a possibility. 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 Comment Actions generate plurals, for now supporting -y ending (-y to -ies/-ys), track generated plurals and show new ones to be checked
Comment Actions My Python knowledge is very limited, but if it runs out of the box, or with very limited required user action with a reasonably new Python (if Python 2 can be reasonably new is another question), I think it would be fine.
Comment Actions 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. Comment Actions 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 Actions use correct python assignment from tuple, ask the user if they want to invoke git, use call instead of check_call to allow testing Comment Actions From my point we can try that one, if there are problems we have plenty of time to revert it.
Comment Actions I think interacting with git or even blocking for input doesn’t feel right I have an issue out on llvm-premerge-checks that would run this tool and that couldn’t block like this Comment Actions 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. Comment Actions 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. Comment Actions Could you just rebase, I'm not getting a clean merge of ClangFormatStyleOptions.rst you are missing the changes from D108752: [clang-format] Group options that pack constructor initializers
Comment Actions remove one print l17, tell me if we don't want the second print (now l17) fixed typo in the script coment -> comment Comment Actions 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 Comment Actions Thanks for the merge, I now understand more how this all works and what we want from these scripts. Comment Actions 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 |
I think these should be sorted.