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

[ci] place all CI helpers under the .ci folder and use - instead of _ in their names #6581

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

StrikerRUS
Copy link
Collaborator

Simplify repo's structure in the following way:

  1. Move all CI scripts under .ci folder
  2. use - instead of _ in scripts' names to ensure their correct ordering

breaking tag due to "public" build-r.R script.

Copy link
Collaborator

@borchero borchero left a comment

Choose a reason for hiding this comment

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

What's the benefit of this change? 😄

@StrikerRUS
Copy link
Collaborator Author

@borchero Simplification of the repo's structure. All helper scripts used in our CI are placed in one directory. Newcomers won't struggle deducing the difference between helpers and .ci folders, for example.

@borchero
Copy link
Collaborator

@StrikerRUS ok, I only briefly looked at the file changes before and it seemed like it would only include renames from snake case to kebab case. I see that it actually moves three files 😄

I guess (1) moving files makes a lot of sense, I'm not too sure about (2) changing the file names but I don't really mind.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

I support the idea of removing helpers/ and just consolidating everything into ci/. That will definitely reduce a bit of friction.

I also support preferring - to _, as it requires one less keypress (it's a minor thing, but still nice!).

However.... I think we should keep build_r.R using snake_case. As you noted, it sort of is "public". It's the documented, preferred entry point for building custom variations of the R package (like with GPU support). I don't support breaking peoples' builds in exchange for a bit more consistency in the naming of scripts.

@StrikerRUS
Copy link
Collaborator Author

@jameslamb

I think we should keep build_r.R using snake_case.

Reverted in d0dbc63.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

great, thank you

@jameslamb jameslamb merged commit 35a2a2e into master Jul 31, 2024
45 checks passed
@jameslamb jameslamb deleted the ci/simplify-structure branch July 31, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants