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

Fix typos #2077

Merged
merged 4 commits into from
Jun 26, 2023
Merged

Fix typos #2077

merged 4 commits into from
Jun 26, 2023

Conversation

szepeviktor
Copy link
Contributor

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jun 26, 2023

Smoke (latest, CodingStyle) started failing 2 days ago. -> #2078

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jun 26, 2023

May I contribute a new CI workflow that regenerates (and commits) generated files?

@mvorisek
Copy link
Member

May I contribute a new CI workflow that regenerates (and commits) generated files?

In past we have been using that, but it turned out more commits are harded to work with, so we have CI to assert all files are up-to-date, but you have to commit them manually from your side - you changed JS files, you need to rebuild them and commit.

@szepeviktor
Copy link
Contributor Author

you changed JS files, you need to rebuild them and commit

Could you please do that for me this time?

@mvorisek
Copy link
Member

In js/ run npm install and npm build.

@szepeviktor
Copy link
Contributor Author

Done.

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jun 26, 2023

It would be much better (no stomach pain for viktor) to set engines in package.json.

{
    "engines": {
        "node": "^18.12",
        "npm": "^9.2.0",
        "yarn": "please-use-npm"
    }
}

And npm install should be npm ci to leave the lock file unchanged.

@mvorisek
Copy link
Member

mvorisek commented Jun 26, 2023

It would be much better (no stomach pain for viktor) to set engines in package.json.

{
    "engines": {
        "node": "^18.12",
        "npm": "^9.2.0",
        "yarn": "please-use-npm"
    }
}

And npm install should be npm ci to leave the lock file unchanged.

we use npm ci in CI, with npm install, did the lock file change?

node / npm - why?

yarn - we should be compatible with yarn

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jun 26, 2023

we use npm ci in CI

Here "ci" does not mean "continuous integration" but clean install.
The goal is to have generated files updated without package versions change.

@szepeviktor
Copy link
Contributor Author

with npm install, did the lock file change?

I was running only npm ci.

@mvorisek mvorisek merged commit a0d07d7 into atk4:develop Jun 26, 2023
21 of 22 checks passed
@szepeviktor szepeviktor deleted the fix-typos branch June 26, 2023 08:54
@mvorisek
Copy link
Member

How viable is to add https://github.com/crate-ci/typos into our CI for atk4/{core, data, ui}? Can we run it without any false positives out of the box?

@szepeviktor
Copy link
Contributor Author

yarn - we should be compatible with yarn

Here is how to do that.

yarn import # import npm's package-lock.json
yarn install --frozen-lockfile

@szepeviktor
Copy link
Contributor Author

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.

None yet

2 participants