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

Add CI linting for some code style issues #1566

Merged
merged 4 commits into from
Aug 6, 2023
Merged

Add CI linting for some code style issues #1566

merged 4 commits into from
Aug 6, 2023

Conversation

speth
Copy link
Member

@speth speth commented Aug 3, 2023

Automate checks for some formatting issues that are easy to miss during pull request reviews.

Changes proposed in this pull request

  • Check modified files for misleading Doxygen comment blocks that do not support "autobrief"
  • Check for whitespace issues using git diff --check: trailing whitespace, tabs in indentation, and CRLF line endings.

If applicable, provide an example illustrating new features this pull request is introducing

CI will indicate failure and logs will include messages like the following for whitespace issues:

include/cantera/kinetics/Kinetics.h:48: trailing whitespace.
+//! reversible with a reverse rate satisfying detailed balance, include  
include/cantera/kinetics/Kinetics.h:55: trailing whitespace.
+//! A kinetics manager implements a kinetics model. Since the model equations  
include/cantera/kinetics/Kinetics.h:56: trailing whitespace.
+//! may be complex and expensive to evaluate, a kinetics manager may adopt 

And like this for bare /*! comment blocks (without a preceding //! block to provide the "brief" description):

------ include/cantera/kinetics/Kinetics.h ------


    /*!
     * Takes as input an array of properties for all species in the mechanism
     * and copies those values belonging to a particular phase to the output
     * array.
     * @param data Input data array.
     * @param phase Pointer to one of the phase objects participating in this
     *     reaction mechanism
     * @param phase_data Output array where the values for the the specified
     *     phase are to be written.
     * @deprecated Unused. To be removed after %Cantera 3.0.
     */

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@speth speth force-pushed the linter branch 4 times, most recently from 176d270 to 0debd0a Compare August 3, 2023 01:51
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #1566 (b7a59f9) into main (3332319) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1566      +/-   ##
==========================================
- Coverage   70.52%   70.50%   -0.02%     
==========================================
  Files         379      379              
  Lines       59110    59110              
  Branches    21232    21232              
==========================================
- Hits        41688    41678      -10     
- Misses      14347    14356       +9     
- Partials     3075     3076       +1     

see 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ischoegl
Copy link
Member

ischoegl commented Aug 3, 2023

#1561 is updated to preempt issues.

@speth speth marked this pull request as ready for review August 6, 2023 02:19
@speth speth added the CI label Aug 6, 2023
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification - so, this looks great from my perspective!

@speth speth merged commit e299379 into Cantera:main Aug 6, 2023
41 of 42 checks passed
@speth speth deleted the linter branch August 6, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants