Closed Bug 1322193 Opened 8 years ago Closed 8 years ago

Verify taskgraph implementations against documentation, with proper regex

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla53

People

(Reporter: Callek, Assigned: hammad13060, Mentored)

References

Details

Attachments

(1 file)

Bug 1322193 - Verify taskgraph implementations against documentation, with proper regex. Updated doc verification for fake values of kinds, parameters etc., regex optimized
58 bytes, text/x-review-board-request
Callek
: review+
Details 
+++ This bug was initially created as a clone of Bug #1302800 +++

Bug 1302800 did a pretty good job. We have a dangling issue though, and that is the regex doesn't match quite right.

`expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]`

Is basically allowing '.+\n' to be enough to say we're good for any given identifier. I suggest `(?: ... )` around both rst header formats, and the `|` leaving the identifier as a "must match". So that we can actually error out when we hit it.

I'm assigning to Hammad (initial patch author) for now, in the hope he's willing to fix this. If it sits before I start to fly back from Hawaii (this friday) I'll likely just patch it and submit myself.

Myself or :dustin, can review.
(In reply to Justin Wood (:Callek) from comment #0)
> +++ This bug was initially created as a clone of Bug #1302800 +++
> 
> Bug 1302800 did a pretty good job. We have a dangling issue though, and that
> is the regex doesn't match quite right.
> 
> `expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in
> identifiers]`
> 
> Is basically allowing '.+\n' to be enough to say we're good for any given
> identifier. I suggest `(?: ... )` around both rst header formats, and the
> `|` leaving the identifier as a "must match". So that we can actually error
> out when we hit it.
> 
> I'm assigning to Hammad (initial patch author) for now, in the hope he's
> willing to fix this. If it sits before I start to fly back from Hawaii (this
> friday) I'll likely just patch it and submit myself.
> 
> Myself or :dustin, can review.

Hmm, thanks Justin for pointing it out. I'll be happy to provide a patch :)
Attachment #8816954 - Flags: review?(bugspam.Callek)
Tests are failing because they mock some `kind types` which are not mentioned in docs. I observed the same behaviour for `parameters`, that's why in Bug 1302800 I moved parameters verification code to decision.py

I think we can move all the doc verification methods to from generator.py to decision.py, but one won't be able to run doc verifications on local machine.
Comment on attachment 8816954 [details]
Bug 1322193 - Verify taskgraph implementations against documentation, with proper regex. Updated doc verification for fake values of kinds, parameters etc., regex optimized

https://reviewboard.mozilla.org/r/97434/#review97838

As to the taskgraph tests failures, my suggestion would be to modify the `fake` kinds to use a magic character, in my preference that would be '_' at the start of the string (generally identified as private idents in most languages) and in the doc check to treat a foo.startswith('_') as "ok to not be docc'd".

Thank you again for the patch!

::: taskcluster/taskgraph/util/verifydoc.py:18
(Diff revision 1)
>      with open(os.path.join(base_path, filename)) as fileObject:
>          doctext = "".join(fileObject.readlines())
>          if appearing_as == "inline-literal":
>              expression_list = ["``" + identifier + "``" for identifier in identifiers]
>          elif appearing_as == "heading":
> -            expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]
> +            expression_list = [identifier + "\n(((-+\n)+)|((.+\n)+))" for identifier in identifiers]

I'd prefer `(?: .. )` instead of `( .. )` here, as the (?: format is non-capturing, so has some slight perf wins.

::: taskcluster/taskgraph/util/verifydoc.py:18
(Diff revision 1)
>      with open(os.path.join(base_path, filename)) as fileObject:
>          doctext = "".join(fileObject.readlines())
>          if appearing_as == "inline-literal":
>              expression_list = ["``" + identifier + "``" for identifier in identifiers]
>          elif appearing_as == "heading":
> -            expression_list = [identifier + "\n[-+\n*]+|[.+\n*]+" for identifier in identifiers]
> +            expression_list = [identifier + "\n(((-+\n)+)|((.+\n)+))" for identifier in identifiers]

Additionally here (and other lines) there are some pep8 issues with regard to line length. I'm not really in favor of the backticks in the error messages, but I don't feel strongly on it one way or the other, so if they stay its still fine to r+ as long as pep8 doesn't fail :-)
Attachment #8816954 - Flags: review?(bugspam.Callek)
Comment on attachment 8816954 [details]
Bug 1322193 - Verify taskgraph implementations against documentation, with proper regex. Updated doc verification for fake values of kinds, parameters etc., regex optimized

https://reviewboard.mozilla.org/r/97434/#review97940

::: taskcluster/taskgraph/util/verifydoc.py:13
(Diff revision 3)
>  
>  base_path = os.path.join(os.getcwd(), "taskcluster/docs/")
>  
>  
>  def verify_docs(filename, identifiers, appearing_as):
>      with open(os.path.join(base_path, filename)) as fileObject:

Possibly add a comment here about why we ignore '_' (hint: for tests), beyond that this whole patch looks good!
Attachment #8816954 - Flags: review?(bugspam.Callek) → review+
Hammad, by the way, you can run a try push from a review request without "publishing" it.  That would help reviewers know when to have another look (when they see the new review request published) while still allowing you to do try runs.

You're doing great work -- keep it up!
This is great, thanks again!
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6702ec08ab43
Verify taskgraph implementations against documentation, with proper regex. Updated doc verification for fake values of kinds, parameters etc., regex optimized r=Callek
https://hg.mozilla.org/mozilla-central/rev/6702ec08ab43
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: