serenity icon indicating copy to clipboard operation
serenity copied to clipboard

LibCore: Refactor elapsed to elapsed_milliseconds function

Open AndyO2 opened this issue 1 year ago • 4 comments

No function change University of Utah: Team Linen

AndyO2 avatar Apr 19 '24 01:04 AndyO2

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why. Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

BuggieBot avatar Apr 19 '24 01:04 BuggieBot

Thanks for resolving this FIXME! However, there's a few things that need to happen before we can merge this change.

The clang-format linter is complaining about the 'unecessary' newlines you removed. Please either run the pre-commit checks locally (https://github.com/SerenityOS/serenity/blob/master/CONTRIBUTING.md#commit-hooks) or run clang-format-16 over the affected files (as seen in the CI logs for the GitHub Actions Build, lint and test jobs).

The commit message seems malformatted. I'm not sure how you managed to prepend your commit title in front of every line of the default commit message prompt, but that needs to be fixed. We use a rebase workflow here, meaning that the commits of each PR are cherry-picked directly onto the master branch instead of being squash-merged like many other projects do. As such, any changes you make to this singular commit PR should be amended (git commit --amend) and force pushed to your branch. This includes any changes to the commit message.

The commit message doesn't quite describe the changes done. The elapsed_milliseconds() function already exists, and the refactor was done in https://github.com/SerenityOS/serenity/commit/197c5729d1e5c20cf1d3f32e7c055eb5f8ee38e8. This change set is the second half of this refactor that Nico did, moving all the callers to use the new name.

ADKaster avatar Apr 19 '24 05:04 ADKaster

Corrected commit message: LibCore: Refactor elapsed to elapsed_milliseconds function #24019

Implemented the refactor mentioned by Nico in 197c572. Changed all remaining instances of elapsed() to elapsed_miliseconds() in order to improve codebase clarity.

Changes to be committed: modified: Userland/Applications/Assistant/Providers.cpp modified: Userland/Applications/CrashReporter/main.cpp modified: Userland/Applications/MouseSettings/DoubleClickArrowWidget.cpp modified: Userland/DevTools/Profiler/main.cpp modified: Userland/Libraries/LibCore/ElapsedTimer.h modified: Userland/Libraries/LibSoftGPU/Device.cpp modified: Userland/Libraries/LibWeb/HTML/Scripting/ClassicScript.cpp

lincknow avatar Apr 19 '24 19:04 lincknow

@lincknow I don't think you understood my request to "change the commit message". In this project we like to follow a rebase workflow. That means that commits from pull requests are directly cherry-picked onto the master branch. As such, the actual commit message of the commits on the pull request are important. Not the PR description, nor a comment on the PR, but the commit.

That means that you should git commit --amend the branch that you have locally, and change the message. After you've updated the commit message, you should git push --force the fixed up commit to your branch (the same one that this PR is using already). It's not necessary to include the files modified in the commit message, as that's already part of the diff. Including the PR # in the commit message is also not required (or desired) as that information is already tracked by GitHub separately.

A better commit title (the first line of the commit message) might be something like

LibCore: Remove obsolete ElapsedTimer::elapsed() function

With the body of the commit message you posted above as a reasonable explaination of the changes.

ADKaster avatar Apr 22 '24 20:04 ADKaster

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Jun 02 '24 07:06 stale[bot]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

stale[bot] avatar Jun 19 '24 02:06 stale[bot]