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

Linting #378

Merged
merged 19 commits into from Apr 27, 2023
Merged

Linting #378

merged 19 commits into from Apr 27, 2023

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Apr 27, 2023

Adds new linters, adds a changed file only listing action, deals with linting issues across the package (aside from nested ifelse - making a help wanted issue), adds an action for rendering the README, cleans up install instructions, goes quietly mad in a corner.

@seabbs seabbs requested a review from sbfnk April 27, 2023 14:29
@sbfnk
Copy link
Contributor

sbfnk commented Apr 27, 2023

This is going to mess with ongoing deveopment in open PRs (especially #363) - might be worth waiting for that and then running the linter?

@seabbs
Copy link
Contributor Author

seabbs commented Apr 27, 2023

This is going to mess with ongoing deveopment in open PRs (especially #363) - might be worth waiting for that and then running the linter?

I did this manually. I am happy to deal with merge issues (also manually) what a dream.

I think we need to get the code base in a nice clean place to enable rapid iteration. Having good linting etc will help avoid building in more technical debt. Especially if others are going to make contributions.

@sbfnk
Copy link
Contributor

sbfnk commented Apr 27, 2023

I did this manually. I am happy to deal with merge issues (also manually) what a dream.

Ah it's fine I'll manage I'm sure. Just might involve some cursing.

I think we need to get the code base in a nice clean place to enable rapid iteration. Having good linting etc will help avoid building in more technical debt. Especially if others are going to make contributions.

Completely agree.

@seabbs seabbs added this pull request to the merge queue Apr 27, 2023
Merged via the queue into main with commit 7f7559b Apr 27, 2023
3 of 11 checks passed
@seabbs
Copy link
Contributor Author

seabbs commented Apr 27, 2023

okay I was not expecting the merge queue to do that...

@seabbs
Copy link
Contributor Author

seabbs commented Apr 27, 2023

I assumed it would wait for checks 😭

@seabbs seabbs deleted the linting branch April 27, 2023 15:51
@github-actions
Copy link

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 9bc622a is merged into main:

  • ❗🐌default: 53s -> 5.57m [+457.36%, +603.41%]
  • ❗🐌no_delays: 44.6s -> 4.5m [+432.73%, +577.94%]
  • ❗🐌random_walk: 15.6s -> 30.1s [+75.09%, +112.32%]
  • ❗🐌stationary: 37.2s -> 3.71m [+357.31%, +640.01%]
  • ❗🐌uncertain: 55s -> 5.74m [+392.33%, +661.5%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@Bisaloo
Copy link
Member

Bisaloo commented Apr 27, 2023

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 9bc622a is merged into main:

  • ❗🐌default: 53s -> 5.57m [+457.36%, +603.41%]

  • ❗🐌no_delays: 44.6s -> 4.5m [+432.73%, +577.94%]

  • ❗🐌random_walk: 15.6s -> 30.1s [+75.09%, +112.32%]

  • ❗🐌stationary: 37.2s -> 3.71m [+357.31%, +640.01%]

  • ❗🐌uncertain: 55s -> 5.74m [+392.33%, +661.5%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

This is likely not correct and just due to the update to R 4.3.0 and the fact that the entire package cache needed to be rebuilt.

seabbs added a commit that referenced this pull request Apr 27, 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.

None yet

3 participants