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

Split CI job. Run formatting task on nightly. #278

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

SLASHLogin
Copy link
Contributor

set formatting task to use nightly toolchain, create new task style-check, and split CI job into build, test, and Clippy - make them depend on style-checks

#194

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Merging #278 (a831627) into main (bfcc550) will increase coverage by 0.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
+ Coverage   82.50%   82.73%   +0.22%     
==========================================
  Files          36       36              
  Lines        7301     7360      +59     
==========================================
+ Hits         6024     6089      +65     
+ Misses       1277     1271       -6     

see 3 files with indirect coverage changes

@orhun
Copy link
Member

orhun commented Jun 25, 2023

What is the reasoning behind using the nightly toolchain for formatting?

@a-kenji
Copy link
Contributor

a-kenji commented Jun 25, 2023

The current rustfmt.toml specifies nightly only features.

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Generally approve this change. Using the new install action is a nice to have that will probably speed up installing cargo-make however.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Added as suggestions so I can commit them straight up and check the speed increase

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Fix typo in suggestions

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

ugh - space

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@joshka
Copy link
Member

joshka commented Jun 25, 2023

Getting yaml perfectly right in the browser is fiddly (spacing matters), so I pulled it down and made the changes locally. I've force pushed to your branch - which should be correct now. I updated the commit message to fit the conventional commit style too.

@joshka
Copy link
Member

joshka commented Jun 25, 2023

Install time drops to 1-2s on Mac / linux and 6-8s on window from 2-4 minutes previously. (compare https://github.com/tui-rs-revival/ratatui/actions/runs/5372360022/jobs/9745700496?pr=277)

@joshka joshka self-requested a review June 25, 2023 23:47
@SLASHLogin
Copy link
Contributor Author

Install time drops to 1-2s on Mac / linux and 6-8s on window from 2-4 minutes previously. (compare https://github.com/tui-rs-revival/ratatui/actions/runs/5372360022/jobs/9745700496?pr=277)

That's great

@joshka joshka enabled auto-merge June 25, 2023 23:52
@joshka joshka self-requested a review June 26, 2023 00:44
@joshka
Copy link
Member

joshka commented Jun 26, 2023

The PR was merged into install-action, so I bumped this to:

      - name: Install cargo-make
        uses: taiki-e/install-action@cargo-make

- Split CI into build, clippy and test.
- Run format on nightly only due to the settings being unstable
@joshka joshka added this pull request to the merge queue Jun 26, 2023
Merged via the queue into ratatui:main with commit 6bdb97c Jun 26, 2023
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.

4 participants