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 workflow #11

Merged
merged 15 commits into from
Jan 14, 2025
Merged

Add CI workflow #11

merged 15 commits into from
Jan 14, 2025

Conversation

abcsds
Copy link
Contributor

@abcsds abcsds commented Dec 30, 2024

Hi Clemens,
I'd like to propose moving up to a v1. The package is covering its intended function, and the goals in the README seem like features to me. What do you think?

This pr adds a CI workflow that will test on push to the main branch. Tests are passing on v1.6, 1.11, and pre, and the bage is in the README. I also added a formatting style.

You maybe already have the keys setup for the tagbot, but these need the DOCUMENTER_KEY variable. This is my reference: https://m3g.github.io/JuliaNotes.jl/stable/publish_docs/#Use-DocumenterTools-to-generate-the-keys This is needed for the CompatHelper that will help us keep our dependencies updated and tested.

@abcsds
Copy link
Contributor Author

abcsds commented Dec 30, 2024

#7

@cbrnr
Copy link
Owner

cbrnr commented Jan 7, 2025

Hi @abcsds, thanks for your PR! These are many changes, so would you mind splitting it up into two parts (one for the style, and another for the CI changes)? We'll see about the new version, I have to go over the code before deciding that. Meanwhile, you can use ## [UNRELEASED] - YYYY-MM-DD in the changelog.

@abcsds
Copy link
Contributor Author

abcsds commented Jan 8, 2025

No problem. Used the chance to add the dejitter function. It's outside the read_xdf function call, so it can be simply used independently. I added a test for it too.

Please consider this PR as a roadmap for v1.0. I'm trying to reproduce #10, and I'd be glad to fix it if I can.

@abcsds
Copy link
Contributor Author

abcsds commented Jan 8, 2025

We do not have issue xdf-modules/libxdf#19, I added tests for it.

@cbrnr
Copy link
Owner

cbrnr commented Jan 8, 2025

You probably missed that part in my comment, but it would be much easier to review if you split up this PR into multiple PRs, each addressing a single issue (so now that you've added dejittering these would be three now 😄).

@abcsds
Copy link
Contributor Author

abcsds commented Jan 8, 2025

I thought you meant to split into commits; you can review each commit independently. I think this adds unnecessary complexity, but here it is:

@abcsds abcsds mentioned this pull request Jan 8, 2025
Closed
.gitignore Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Do we really need this? I'd like to keep things as simple as possible, and checking the [compat] section seems simple enough to do manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compatibility helper runs every two weeks and if any of the dependencies has had an update, it will update the project's Manifest.toml, build a PR, and that will trigger the CI tests. This means the package gets tested on a constantly changing environment. It could be done every six months by some going ]update,] test, and build a pr, yes, but removing the human factor avoids long jumps in the dependencies' versions, and possible mistakes. Does it cost for you? I would otherwise recommend keeping it: it will allow the package to be constantly up to date with minimal effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, isn't that the point of Continuous Integration?

Copy link
Owner

Choose a reason for hiding this comment

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

The only entry in the [compat] section is julia = "1". Should we have to add any compatibility constraint in the future, I'm happy to add this action, but currently I'd rather not.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you directly use the example from https://github.com/julia-actions/julia-runtest? Or do you require any of the specific differences in your version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use PkgTemplates: https://juliaci.github.io/PkgTemplates.jl/stable/

What do you mean with their example? the first one on the readme?

Here for comparison:

name: Run tests

on:
  push:
    branches:
      - master
      - main
  pull_request:

# needed to allow julia-actions/cache to delete old caches that it has created
permissions:
  actions: write
  contents: read

jobs:
  test:
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        julia-version: ['lts', '1', 'pre']
        julia-arch: [x64, x86]
        os: [ubuntu-latest, windows-latest, macOS-latest]
        exclude:
          - os: macOS-latest
            julia-arch: x86

    steps:
      - uses: actions/checkout@v4
      - uses: julia-actions/setup-julia@v2
        with:
          version: ${{ matrix.julia-version }}
          arch: ${{ matrix.julia-arch }}
      - uses: julia-actions/cache@v2
      - uses: julia-actions/julia-buildpkg@v1
      - uses: julia-actions/julia-runtest@v1
        # with:
        #   annotate: true

Compared that example, this PR includes:

  • the name was chosen by PkgTemplate
  • tags was added by PkgTemplate, it enables CI in any tag you may add.
  • cancelling of concurrent jobs on PRs is also added by PkgTemplate
  • a timeout of 60m is added.

@cbrnr cbrnr merged commit a870206 into cbrnr:main Jan 14, 2025
3 checks passed
Copy link
Owner

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

Thanks @abcsds!

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.

2 participants