Skip to content

Conversation

@davschneller
Copy link
Contributor

  • Periodic mesh support for GMSH, msh2 and msh4.
  • Format everything; add a pre-commit config
  • Shorten and add the license headers akin to SeisSol (with extra treatment for the third_party folder).

@davschneller davschneller marked this pull request as ready for review October 11, 2025 22:37
@Thomas-Ulrich
Copy link
Contributor

Thomas-Ulrich commented Oct 15, 2025

  1. node version

with kubuntu 24, I get:

[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/home/ulrich/.cache/pre-commit/repo7ogbkjjg/node_env-system/bin/node', '/usr/bin/npm', 'install', '--include=dev', '--include=prod', '--ignore-prepublish', '--no-progress', '--no-save')
return code: 1
stdout: (none)
stderr:
    npm ERR! code EBADENGINE
    npm ERR! engine Unsupported engine
    npm ERR! engine Not compatible with your version of node/npm: markdownlint-cli2@0.18.1
    npm ERR! notsup Not compatible with your version of node/npm: markdownlint-cli2@0.18.1
    npm ERR! notsup Required: {"node":">=20"}
    npm ERR! notsup Actual:   {"npm":"9.2.0"}
    
    npm ERR! A complete log of this run can be found in:
    npm ERR!     /home/ulrich/.npm/_logs/2025-10-15T12_06_49_001Z-debug-0.log
Check the log at /home/ulrich/.cache/pre-commit/pre-commit.log

maybe it could be worth using an older rev v0.13.0?
(given that ubuntu 24 is the LTS, not that old).

EDIT: could update nvm with but do we want to force developers to do this update for a linter?

# Install nvm if not installed
curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.7/install.sh | bash
source ~/.bashrc

# Install Node 20
nvm install 20
nvm use 20
  1. Python version

Is this language_version specifier really necessary?
language_version: python3.13
I mean, we do not always have such a recent Python installed.

Copy link
Contributor

@Thomas-Ulrich Thomas-Ulrich left a comment

Choose a reason for hiding this comment

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

LGTM. See also the 2 comments about precommits.

@davschneller
Copy link
Contributor Author

Thanks for the feedback!

Regarding the pre-commit comments:

  • node version: I tried to reproduce it on an Ubuntu 24.04 without a present node install; it worked there. Not quite sure what to suggest here tbh, as Ubuntu 24.04 should be able to somehow manage the node install I think. Otherwise—yeah, we can downgrade.
  • Python version: yes; I'll remove it. (the pre-commit hook doesn't take care of that by itself somehow)

(all of these changes to the pre-commit config will also need to be propagated to SeisSol and a PSpaMM PR probably)

@Thomas-Ulrich
Copy link
Contributor

ok then forget about the node version. maybe it is because I did several dist-upgrade instead of a new installation.

@davschneller davschneller merged commit 6518bc6 into master Oct 19, 2025
3 checks passed
@davschneller davschneller deleted the davschneller/periodic branch October 19, 2025 22:30
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