Skip to content

Conversation

6543
Copy link
Member

@6543 6543 commented Feb 2, 2022

as title

@6543 6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 2, 2022
@6543 6543 added this to the 1.17.0 milestone Feb 2, 2022
@6543 6543 requested a review from zeripath February 2, 2022 13:48
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 2, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 2, 2022

I propose to use only NewCommand because otherwise every method will end in ...Context sometimes. As the context parameter should be the new normal, we could drop the Context suffix.

@6543 6543 changed the title Remove git.NewCommand() replace git.NewCommand() with git.NewCommandContext() Feb 2, 2022
@zeripath
Copy link
Contributor

zeripath commented Feb 2, 2022

Also need to get rid of the commands like CloneWithContext etc.

@6543
Copy link
Member Author

6543 commented Feb 3, 2022

@zeripath can not find CloneWithContext and I think it's for a next pull :)

@zeripath
Copy link
Contributor

zeripath commented Feb 3, 2022

Looks like I might have fixed that in the previous pr.

@lunny
Copy link
Member

lunny commented Feb 3, 2022

I propose to use only NewCommand because otherwise every method will end in ...Context sometimes. As the context parameter should be the new normal, we could drop the Context suffix.

I think so. NewCommand is a better name than NewCommandContext.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 4, 2022
@6543
Copy link
Member Author

6543 commented Feb 4, 2022

@lunny already changed :D

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 6, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 6, 2022

Title should be changed.

@6543 6543 changed the title replace git.NewCommand() with git.NewCommandContext() Delete old git.NewCommand() and use it as git.NewCommandContext() Feb 6, 2022
@6543
Copy link
Member Author

6543 commented Feb 6, 2022

@KN4CK3R suggestions for a better one are welcome

@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 6, 2022

I would just have switched both methods but the current one is ok too. 👍

replace git.NewCommandContext() with git.NewCommand()

@codecov-commenter
Copy link

Codecov Report

Merging #18552 (3943c22) into main (8ae5e6d) will decrease coverage by 0.00%.
The diff coverage is 60.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #18552      +/-   ##
==========================================
- Coverage   46.30%   46.29%   -0.01%     
==========================================
  Files         846      846              
  Lines      121241   121239       -2     
==========================================
- Hits        56140    56129      -11     
- Misses      58289    58298       +9     
  Partials     6812     6812              
Impacted Files Coverage Δ
cmd/hook.go 7.11% <0.00%> (ø)
modules/doctor/mergebase.go 10.25% <0.00%> (ø)
modules/doctor/misc.go 21.55% <0.00%> (ø)
modules/git/pipeline/lfs_nogogit.go 0.00% <0.00%> (ø)
modules/git/pipeline/namerev.go 0.00% <0.00%> (ø)
modules/indexer/code/bleve.go 63.36% <0.00%> (ø)
modules/indexer/code/elastic_search.go 1.12% <0.00%> (ø)
routers/private/hook_verification.go 0.00% <0.00%> (ø)
routers/web/repo/pull.go 30.63% <0.00%> (ø)
services/asymkey/sign.go 38.46% <0.00%> (ø)
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ae5e6d...3943c22. Read the comment docs.

@6543 6543 merged commit 3043eb3 into go-gitea:main Feb 6, 2022
@6543 6543 deleted the NO-git.NewCommand branch February 6, 2022 19:01
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 8, 2022
* giteaofficial/main: (28 commits)
  Added auto-save whitespace behavior if it changed manually (go-gitea#15566)
  Support custom ACME provider (go-gitea#18340)
  Refactor i18n, use Locale to provide i18n/translation related functions (go-gitea#18648)
  Only request write when necessary (go-gitea#18657)
  [skip ci] Updated translations via Crowdin
  Add separate SSH_USER config option (go-gitea#17584)
  Be more lenient with label colors (go-gitea#17752)
  remove redundant call to UpdateRepoStats during migration (go-gitea#18591)
  more repo dump/restore tests, including pull requests (go-gitea#18621)
  No longer show the db-downgrade SQL in production (go-gitea#18653)
  Fix the missing i18n key for update checker (go-gitea#18646)
  Update gitea-vet (go-gitea#18640)
  Future proof for 1.18 (go-gitea#18644)
  Add `contrib/upgrade.sh` (go-gitea#18286)
  If rendering has failed due to a net.OpError stop rendering (go-gitea#18642)
  Delete old git.NewCommand() and use it as git.NewCommandContext() (go-gitea#18552)
  Update JS dependencies (go-gitea#18636)
  fix commits_list_small.tmpl (go-gitea#18641)
  Fix `make fmt` and `make fmt-check` (go-gitea#18633)
  Frontport of changelog for v1.16.1 (go-gitea#18615)
  ...
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants