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.

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

Check for FIXME etc. during CI #119

Merged
merged 9 commits into from
Feb 28, 2023
Merged

Check for FIXME etc. during CI #119

merged 9 commits into from
Feb 28, 2023

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented Jan 17, 2023

Add a CI check for FIXME so that code with FIXMEs in it can't pass CI checks, proc.review, or proc.merge. The purpose is to allow devs to use FIXME to mark things that must be fixed in development branches before merging.

See also #140 (comment)

@rptb1 rptb1 added the nice Little impact; only do if low cost label Jan 17, 2023
@rptb1 rptb1 changed the title Check for FIXME etc. as part of CI Check for FIXME etc. during CI Jan 17, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 20, 2023

Long ago, before the FIXME convention became popular, the MPS used "@@@@" to do something similar. There are still about 80 of those in the code, but they are low risk and not dealt with by this branch (see purpose above).

@rptb1 rptb1 requested review from thejayps and UNAA008 February 20, 2023 09:02
@rptb1 rptb1 marked this pull request as ready for review February 20, 2023 09:03
@rptb1 rptb1 added the low risk This work is or would be of low risk of introducing defects. label Feb 20, 2023
@thejayps thejayps added the optional Will cause failures / of benefit. Worth assigning resources. label Feb 20, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Executing proc.review.entry

  1. Start time 06:42.
  2. Change is small and low risk but nobody is available for express review. Plan to batch with other similar changes on 2023-02-23.
  3. proc.review.entry.criteria: Applying entry.universal and entry.impl
  4. entry.universal.reason: Pull request describes reason, and links to an issue about why there isn't a better documented reason!
  5. Entry passed.
  6. Entry took 5 mins.

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Executing proc.review.plan

  1. Start time 06:47.
  2. proc.review.plan.time: Change combines two things: a new script with CI, and discharging about 10 FIXMEs. Each one of the FIXMEs might take a couple of minutes to check. Total checking time about 30 mins.
  3. proc.review.plan.role: @thejayps could check CI and scripts for correctness. @UNAA008 check FIXME changes. @rptb1 checks convention and clarity.
  4. proc.review.plan.schedule: @UNAA008 @thejayps @rptb1 will review on 2023-02-23 at 11:00.
  5. proc.review.plan.homework: @thejayps should read https://docs.github.com/en/actions/using-workflows/about-workflows
  6. Planning took 10 mins.

@rptb1 rptb1 removed the nice Little impact; only do if low cost label Feb 22, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 23, 2023

Executing proc.review.kickoff

  1. Start time 11:08.
  2. Logging will be 11:45 latest but I'll see how people are doing at about 11:30.

Copy link
Member Author

@rptb1 rptb1 left a comment

Choose a reason for hiding this comment

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

Executing proc.review.check

  1. Start time 11:16
  2. 4 minor defects
  3. End time 11:29

tool/check-fixme Outdated Show resolved Hide resolved
tool/check-fixme Outdated Show resolved Hide resolved
tool/check-fixme Show resolved Hide resolved
code/config.h Outdated Show resolved Hide resolved
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.

The general principle followed to convert FIXME to TODO worked well.
I had 2 comments requesting clarification inserted inline.

code/config.h Outdated Show resolved Hide resolved
code/splay.c Outdated Show resolved Hide resolved
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.

m https://memory-pool-system.readthedocs.io/en/latest/design/guide.developer.html Doesn't talk about the difference between FIXME and TODO

M https://memory-pool-system.readthedocs.io/en/latest/design/guide.review.html seems to be deprecated. Danger that develepers will pass reviews based on this doc

q Where do we state that CI will fail if fixme's are found, will they fail if fixmes are found

q Why is the job in fixme-check.yml called check-rst? what has this to do specifically with restructured text?

@rptb1
Copy link
Member Author

rptb1 commented Feb 23, 2023

Executing proc.review.log

  1. Start time 11:31
  2. @UNAA008 found 2 minor, @thejayps find 1 M, 1 m, 2 q, @rptb1 4 m.

m https://memory-pool-system.readthedocs.io/en/latest/design/guide.developer.html Doesn't talk about the difference between FIXME and TODO

I. This is already logged as #140 (comment) . But let's add a pointer to that guide.

q Where do we state that CI will fail if fixme's are found, will they fail if fixmes are found

m. The YAML file should probably link to the more specific docs about CI in Workflows.

q Why is the job in fixme-check.yml called check-rst? what has this to do specifically with restructured text?

m. That's a mistake.

  1. Logging complete at 11:54.

tool/check-fixme Outdated Show resolved Hide resolved
@rptb1 rptb1 mentioned this pull request Feb 23, 2023
@rptb1 rptb1 self-assigned this Feb 23, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 24, 2023

m https://memory-pool-system.readthedocs.io/en/latest/design/guide.developer.html Doesn't talk about the difference between FIXME and TODO

Raise: Already part of #140 (comment)

M https://memory-pool-system.readthedocs.io/en/latest/design/guide.review.html seems to be deprecated. Danger that develepers will pass reviews based on this doc

Raise: #123 (comment)

q Where do we state that CI will fail if fixme's are found, will they fail if fixmes are found

Fix: Updated link in 38d9feb

q Why is the job in fixme-check.yml called check-rst? what has this to do specifically with restructured text?

Fix: Corrected mistake in 38d9feb

@rptb1
Copy link
Member Author

rptb1 commented Feb 28, 2023

Executing proc.review.exit

  1. Start time 12:54.
  2. Revised change passed.
  3. review.exit.calc:
  • hours used: 90 minutes
  • hours saved: 2 hours fixing review procedure (and the manual)
  • major defects remaining: 0

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.

proc.review.exit.pass

@rptb1 rptb1 merged commit f297ada into master Feb 28, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 28, 2023

Executing proc.merge.pull-request

  1. Start time 15:20
  2. There is no issue attached, but the PR explains why it's helpful, and there is pending doc about it MPS documentation conventions aren't well documented #140 (comment) .
  3. Automated test is not feasible as it would need to introduce FIXMEs in Git and watch CI.
  4. End time 15:25.
  5. Merge took 5 mins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low risk This work is or would be of low risk of introducing defects. optional Will cause failures / of benefit. Worth assigning resources.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants