Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Verify taskgraph implementations against documentation, with proper regex

RESOLVED FIXED in mozilla53

Status

Taskcluster
Task Configuration
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: Callek, Assigned: HAMMAD AKHTAR, Mentored)

Tracking

unspecified
mozilla53

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
+++ 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.
(Assignee)

Comment 1

8 months ago
(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 :)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8816954 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 3

8 months ago
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.
(Reporter)

Comment 4

8 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 7

8 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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!
(Reporter)

Comment 11

8 months ago
This is great, thanks again!

Comment 12

8 months ago
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

Comment 13

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6702ec08ab43
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.