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

[minor-refactor] Remove unused title parameter on merge request creation helper. #7379

Merged
merged 1 commit into from Oct 6, 2014

Conversation

4 participants
@cirosantilli
Copy link
Contributor

commented Jul 26, 2014

Every new merge request path will pass through the new action:

, which in turn uses MergeRequests::BuildService, which fixes the title to a humanized branch name no matter what :title parameter you pass:
merge_request.title = merge_request.source_branch.titleize.humanize

@TeatroIO

This comment has been minimized.

Copy link

commented Jul 26, 2014

I've prepared a stage. Click to open.

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2014

Thanks for your interest in improving the GitLab codebase! Please update your merge request according to the contributing guidelines.

Remove unused title parameter.
It is always overridden by to humanized source branch name by
MergeRequests::BuilService, which is used by new_project_merge_request_path.
@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2014

@jvanbaarsen trivial rebased. Is this all that needs to be done?

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2014

@cirosantilli If you can explain a little better what this MR does, since its not all that clear to me (sorry :( )

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2014

@jvanbaarsen No problem!

The :title parameter is never used so I removed it. It is always set by https://github.com/gitlabhq/gitlabhq/blob/master/app/services/merge_requests/build_service.rb#L20, which completely ignores it (but sets it to the same value as passed merge_request.source_branch.titleize.humanize).

As it stands, it duplicates the default value, and is misleading as someone might think the parameter has an effect when creating a new function with a different value.

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2014

@cirosantilli Ok thanks!
@randx Looks good to merge

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2014

ping @randx

dzaporozhets added a commit that referenced this pull request Oct 6, 2014

Merge pull request #7379 from cirosantilli/remove-title
[minor-refactor] Remove unused title parameter on merge request creation helper.

@dzaporozhets dzaporozhets merged commit 274e795 into gitlabhq:master Oct 6, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details

@cirosantilli cirosantilli deleted the cirosantilli:remove-title branch Oct 6, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.