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

Unconditionally format generated code but in CI #1085

Closed
wants to merge 1 commit into from

Conversation

tristan0x
Copy link
Member

Rely on utility cmake/hpc-coding-conventions/bin/format

Rely on utility `cmake/hpc-coding-conventions/bin/format`
@tristan0x tristan0x force-pushed the tristan0x/always-format-jinja branch from 0e5cd27 to d41d501 Compare September 28, 2023 16:17
@tristan0x tristan0x changed the title src/language: unconditionally format generated code Unconditionally format generated code but in CI Sep 28, 2023
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (10bf2d2) 70.18% compared to head (d41d501) 88.61%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1085       +/-   ##
===========================================
+ Coverage   70.18%   88.61%   +18.42%     
===========================================
  Files         188      169       -19     
  Lines       25627    12474    -13153     
===========================================
- Hits        17987    11054     -6933     
+ Misses       7640     1420     -6220     

see 24 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot
Copy link
Collaborator

Logfiles from GitLab pipeline #157028 (:white_check_mark:) have been uploaded here!

Status and direct links:

@tristan0x tristan0x marked this pull request as ready for review September 29, 2023 08:05
@tristan0x tristan0x requested review from pramodk and ohm314 September 29, 2023 08:06
Copy link
Contributor

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

Just few clarification questions otherwise LGTM

@@ -25,14 +24,20 @@

LOGGING_FORMAT = "%(levelname)s:%(name)s: %(message)s"
LOGGER = logging.getLogger("NMODLCodeGen")
CLANG_FORMAT_EXECUTABLE = (
Copy link
Contributor

Choose a reason for hiding this comment

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

a pure nitpick: it's not really aclang-format executable?

Copy link
Contributor

Choose a reason for hiding this comment

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

CLANG_FORMAT_COMMAND then :)

Comment on lines +104 to +111
@property
def within_ci(self):
"""Return True if code is running in a continuous-integration environment, else otherwise"""
for varname in ["CI", "CI_PIPELINE_ID", "AZURE_ENV_NAME"]:
if varname in os.environ:
return True
return False

Copy link
Contributor

Choose a reason for hiding this comment

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

wondering why we need it i.e. what happens if the files are formatted in CI as well? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to understand this but couldn't quite figure it out from looking at our current CMake files and CI scripts...

if self.language == "c++" and self.app.clang_format:
LOGGER.debug("Formatting C++ file %s", str(file))
subprocess.check_call(self.app.clang_format + ["-i", str(file)])
if self.language == "c++" and not self.app.within_ci:
Copy link
Contributor

Choose a reason for hiding this comment

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

A clarification question: if user doesn't have "compatible" clang format installed on his/her machine, what happens in this case? .i.e. if there is no clang-format installed or of a different version than we support, user will still be able to build the nmodl without any error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is taken care of from within the coding convention tool that wraps clang-format?

@1uc 1uc closed this Feb 26, 2024
@1uc 1uc deleted the tristan0x/always-format-jinja branch July 12, 2024 14:03
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.

5 participants