GNOME Bugzilla – Bug 670985
[doc-tool] mishandles --output arguments without trailing slashes
Last modified: 2015-03-02 19:52:42 UTC
I found that g-ir-doc-tool treats '--output /foo/bar' differently to '--output /foo/bar/': the former puts the generated files into /foo. Patch fixing this to follow, preceded by a number of miscellaneous clean-up patches.
Created attachment 208602 [details] [review] annotationparser: remove duplication of annotation names Previously, the validation code compared each option name to a constant (such as OPT_ALLOW_NONE) which expands to the string used in a source file, and if they matched it would typically pass that same string to _validate_option(). But the annotation name was written out longhand each time, which seemed strange to me.
Created attachment 208603 [details] [review] annotationparser: correct "maximium" in error messages "Maximium" is not an English word. Even if corrected to "maximum" (which is), "at maximum" is not conventional usage: "at most" is more idiomatic.
Created attachment 208604 [details] [review] annotationparser: split validate() into sub-methods Most cases in validate() were already simply calls to _validate_option() with particular arguments; this extracts the code from the remaining options to their own methods, making the dispatch table more legible.
Created attachment 208605 [details] [review] configure: correct --enable-doctool documentation The executable is named 'g-ir-doc-tool', not 'g-ir-doctool'.
Created attachment 208606 [details] [review] doctool: improve error when --output isn't a directory
Created attachment 208608 [details] [review] doctool: handle --output dirs without trailing slashes Previously, passing --output /foo/bar/ to g-ir-doc-tool would place the generated documentation in /foo/bar, as expected, but passing --output /foo/bar would place the generated documentation in /foo. This is because os.path.dirname() essentially just splits on the last '/' in the string and returns everything before it. It does not actually hit the filesystem. Meanwhile, os.path.join() correctly deals with trailing slashes: os.path.join('/foo/', 'bar') and os.path.join('/foo', 'bar') are equivalent.
Review of attachment 208602 [details] [review]: Good catch.
Review of attachment 208603 [details] [review]: Great.
Review of attachment 208604 [details] [review]: Looks great.
Review of attachment 208605 [details] [review]: Sure
Review of attachment 208606 [details] [review]: Yup
Review of attachment 208608 [details] [review]: Okay.
Attachment 208602 [details] pushed as 5030e41 - annotationparser: remove duplication of annotation names Attachment 208603 [details] pushed as 4c38ca6 - annotationparser: correct "maximium" in error messages Attachment 208604 [details] pushed as c8cfa7c - annotationparser: split validate() into sub-methods The rest needs to be rebased/updated for the latest annotationparser changes.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
Created attachment 298343 [details] [review] configure: correct --enable-doctool documentation rebased onto current master...
Review of attachment 208606 [details] [review]: Rejected in it's current form as it no longer applies to current code. These days --output can be either a file or a directory depending on the value of the --write-sections-file parameter.
Review of attachment 208608 [details] [review]: Rejected because already fixed in current code as: output_file_name = os.path.join(os.path.abspath(output), page_id + '.page')
(In reply to Dieter Verfaillie from comment #18) > Created attachment 298343 [details] [review] [review] > configure: correct --enable-doctool documentation > > rebased onto current master... Pushed this as 105852e