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

mmwtex.c: boolean-style evaluation in conditionals (+ linting) #167

Merged
merged 14 commits into from
Feb 5, 2024

Conversation

benjub
Copy link
Collaborator

@benjub benjub commented Oct 29, 2023

Found some inconsistencies while on #160, which I signal as TODOs in this PR. I also made some simplifications (mainly, evaluate boolean-like variables as bools) and linting, all in mmwtex.c.

src/mmwtex.c Outdated Show resolved Hide resolved
src/mmwtex.c Show resolved Hide resolved
src/mmwtex.c Outdated Show resolved Hide resolved
src/mmwtex.c Outdated Show resolved Hide resolved
@benjub benjub requested review from tirix and digama0 November 18, 2023 18:55
@benjub
Copy link
Collaborator Author

benjub commented Feb 4, 2024

@digama0 : following approval by Thierry, can you merge this one ?

src/mmwtex.c Outdated Show resolved Hide resolved
src/mmwtex.c Outdated Show resolved Hide resolved
src/mmwtex.c Outdated Show resolved Hide resolved
src/mmwtex.c Outdated Show resolved Hide resolved
src/mmwtex.c Outdated Show resolved Hide resolved
src/mmwtex.c Outdated Show resolved Hide resolved
src/mmwtex.c Outdated Show resolved Hide resolved
benjub and others added 3 commits February 4, 2024 19:41
Co-authored-by: Mario Carneiro <di.gama@gmail.com>
Co-authored-by: Mario Carneiro <di.gama@gmail.com>
Co-authored-by: Mario Carneiro <di.gama@gmail.com>
@benjub
Copy link
Collaborator Author

benjub commented Feb 4, 2024

I didn't apply the four suggestions changing imperative to indicative since I thought it was the opposite convention, but I'm unsure. I'll let you decide to apply or not before merging.

@digama0
Copy link
Member

digama0 commented Feb 4, 2024

I took a census of how this is used in the .h file comments on all functions. As expected, there has not historically been any consistency about this, but the indicatives seem to be winning:

  • Imperative mood: 73 functions
  • Indicative mood: 94 functions
  • Noun phrase: 31 functions

Doxygen seems to have thrown its opinion into the ring slightly with the \returns keyword. My preference is to standardize on indicative mood, perhaps it should be added as an additional element to the "code cleanup" issue?

benjub and others added 2 commits February 4, 2024 20:52
Co-authored-by: Mario Carneiro <di.gama@gmail.com>
Co-authored-by: Mario Carneiro <di.gama@gmail.com>
benjub and others added 2 commits February 4, 2024 20:52
Co-authored-by: Mario Carneiro <di.gama@gmail.com>
Co-authored-by: Mario Carneiro <di.gama@gmail.com>
@benjub
Copy link
Collaborator Author

benjub commented Feb 4, 2024

perhaps it should be added as an additional element to the "code cleanup" issue?

Done. (Add suggestions applied.)

@digama0 digama0 merged commit e75b336 into metamath:master Feb 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants