Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check the syntax of reStructuredText files during CI #112

Merged
merged 17 commits into from
Feb 10, 2023
Merged

Check the syntax of reStructuredText files during CI #112

merged 17 commits into from
Feb 10, 2023

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented Jan 14, 2023 •
edited
Loading

Intended to fix and prevent #113 .

  1. Adds a shell script to check the syntax of our reStructuredText files.
  2. Adds a GitHub Actions workflow to run that script as part of CI.
  3. Fixes up errors found by the above.

Note that in each case where I changed a tag, I grepped the entire tree for references to that tag.

Test case: What we would be testing is whether developers can introduce errors in reStructuredText to master. That's not practical to automate. The new script here only checks whether such errors are present now.

rptb1 added 13 commits January 14, 2023 00:11
@rptb1
Adding a GitHub CI workflow to check the syntax of reStructuredText f…
6dd7a08 
…iles.
@rptb1
Suppress Travis CI on branch/2023-01-13/rst-check for faster testing …
646436a 
…of GitHub CI.
@rptb1
Run checking steps even if other steps fail.
79fb554
@rptb1
Moving checking to its own script so it can be used at the command li…
a1c003d 
…ne, which also simplifies the GitHub workflow.
@rptb1
Refining pruning rules for command-line use.
db63baf
@rptb1
Fixing basic reStructuredText syntax and reference errors found by to…
5da9822 
…ol/checkrst.
@rptb1
Improving the naming so that results are clearer in GitHub output.
83c1230
@rptb1
Fixing warnings found by shellcheck.
b2ae297
@rptb1
Fixing duplicate tag design.mps.config.var introduced in commit 533f4fa
caa7a60
@rptb1
Oops, forgot to fix up the RST ref in commit caa7a60
1a1d265
@rptb1
Fixing duplicate tags introduced by e9841d2 as a consequence of b0433b3
d5d996d
@rptb1
Fixing duplicate tag design.mps.arena.tract.field.base. Looks like a …
b8d1851 
…copy-paste error.
@rptb1
Fixing duplicate tag design.mps.prmc.if.init.thread. Looks like a cop…
dd6493a 
…y-paste error.
rptb1 referenced this pull request Jan 14, 2023
@gareth-rees
Convert message, pool, poolamc, prot, and vm design documents to reSt…
e9841d2 
…ructuredText.

Copied from Perforce
 Change: 182116
 ServerID: perforce.ravenbrook.com
@rptb1 rptb1 requested review from thejayps and UNAA008 January 14, 2023 10:13
@rptb1 rptb1 linked an issue Jan 14, 2023 that may be closed by this pull request
Various MPS documents contain syntax and reference errors #113
Closed
@rptb1 rptb1 assigned rptb1 and unassigned rptb1 Jan 14, 2023
@rptb1 rptb1 added documentation git-migration Project migration from Ravenbrook internal Perforce infrastructure to public git repo labels Jan 14, 2023
@rptb1 rptb1 marked this pull request as ready for review January 14, 2023 10:28
This was referenced Jan 14, 2023
Merged
Merged
@rptb1 rptb1 mentioned this pull request Jan 15, 2023
Merged
@rptb1 rptb1 added the optional Will cause failures / of benefit. Worth assigning resources. label Jan 15, 2023
@thejayps
Copy link
Contributor

@UNAA008 and @thejayps review discussion

UNAA008
UNAA008 reviewed Jan 24, 2023
View reviewed changes
Copy link
Contributor

@UNAA008 UNAA008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is unusual in that it combines a change to the checking machinery with consequential changes to source files. It makes sense to do this in this instance but in general we would require separate Pull Requests for distinct change sets.

UNAA008
UNAA008 reviewed Jan 24, 2023
View reviewed changes
.travis.yml Show resolved Hide resolved
UNAA008
UNAA008 requested changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@UNAA008 UNAA008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to resolve the comments raised before we appprove this.

thejayps
thejayps previously requested changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@thejayps thejayps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Joint review with @UNAA008.

@Ravenbrook Ravenbrook deleted a comment from Ravenbot Jan 25, 2023
@rptb1
Copy link
Member Author

rptb1 commented Jan 25, 2023 •
edited
Loading

This PR is unusual in that it combines a change to the checking machinery with consequential changes to source files. It makes sense to do this in this instance but in general we would require separate Pull Requests for distinct change sets.

You may find that it's not unusual. I've done the same thing every time I introduced checking: #117 #107 . Otherwise we break master with the checks. So I'm not sure I agree.

@rptb1 rptb1 closed this Jan 25, 2023
@rptb1 rptb1 reopened this Jan 25, 2023
@rptb1
Copy link
Member Author

rptb1 commented Jan 25, 2023 •
edited
Loading

I would like to resolve the comments raised before we appprove this.

Please take a look. Then consider executing https://github.com/Ravenbrook/mps/blob/branch/2023-01-07/pull-request-merge-procedure/procedure/pull-request-merge.rst . Let me know whether what you decide to do.

@rptb1 rptb1 requested a review from UNAA008 January 25, 2023 21:12
UNAA008
UNAA008 approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@UNAA008 UNAA008 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to resolve the comments raised before we appprove this.

I think this is now ready to merge.

@rptb1 rptb1 dismissed thejayps’s stale review February 7, 2023 16:08

Empty review that requests changes. No action possible.

@UNAA008
Copy link
Contributor

UNAA008 commented Feb 10, 2023

Executing [proc.merge.pull-request] using
https://github.com/Ravenbrook/mps/blob/40f23128c2bcddd17aadc0c4c3369e6473c754a1/procedure/pull-request-merge.rst

  1. There is no automated test case to demonstrate the problem being fixed. To make one would involve introducing errors purely for demonstration.

It took about an hour get to starting the actual merge procedure, which included time spent on process improvement for the documentation in section 4. The prerequisite steps were carried out previously.
Stalled at step 3 of the merge procedure ('git pull --ff-only perforce master') due to authentication difficulties connected with PNJ's idiosyncratic Windows practises.
About 50 minutes was spent investigating those issues and the merge is suspended at this point pending further research.

@UNAA008 UNAA008 merged commit 8a88acd into master Feb 10, 2023
@UNAA008 UNAA008 mentioned this pull request Feb 10, 2023
Closed
@UNAA008
Copy link
Contributor

UNAA008 commented Feb 10, 2023

Merge procedure recommenced at 16:42.
Merge committed at 17:57 (1h15 after the procedure restarted)
Total time to this point 2h05.
Then an additional 20 minutes working on step 9 and the outcome of the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UNAA008 UNAA008 UNAA008 approved these changes

@thejayps thejayps thejayps left review comments

Assignees
No one assigned
Labels
documentation git-migration Project migration from Ravenbrook internal Perforce infrastructure to public git repo optional Will cause failures / of benefit. Worth assigning resources.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Various MPS documents contain syntax and reference errors
3 participants
@rptb1 @thejayps @UNAA008