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

CHE-2456: Svn switch action #2758

Merged
merged 13 commits into from
Oct 13, 2016
Merged

CHE-2456: Svn switch action #2758

merged 13 commits into from
Oct 13, 2016

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Oct 11, 2016

What does this PR do?

Add svn switch action.

What issues does this PR fix or reference?

#2456

New behavior

Ability to switch to trunk/branch/tag from svn menu

@codenvy-ci
Copy link

Build # 686 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/686/ to view the results.

@tolusha
Copy link
Contributor Author

tolusha commented Oct 11, 2016

@vinokurig
@vzhukovskii
@benoitf
@vparfonov
@sleshchenko
pls review

Copy link
Contributor

@vinokurig vinokurig left a comment

Choose a reason for hiding this comment

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

@codenvy-ci
Copy link

Build # 687 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/687/ to view the results.

@bmicklea bmicklea added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Oct 11, 2016
protected void onClose() { }

@UiHandler("switchToBranch")
public void onSwitchToBranchClicked(final ClickEvent event) { delegate.onSwitchToBranchChanged(); }
Copy link
Member

Choose a reason for hiding this comment

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

Please format this method body in the same way to other

public void onWorkingCopyDepthChanged(final ChangeEvent event) {
delegate.onWorkingCopyDepthChanged();
}

Copy link
Member

Choose a reason for hiding this comment

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

remove extra lines

* @param request
* the switch request
* @return the response
* @throws SubversionException
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it throws ApiException

*
* @return the response containing target children
* * @param request
Copy link
Member

Choose a reason for hiding this comment

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

remove extra *

request.getUsername(),
request.getPassword());

return DtoFactory.getInstance().createDto(ListResponse.class)
Copy link
Member

Choose a reason for hiding this comment

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

DtoFactory.newDto(ListResponse)... ?

@codenvy-ci
Copy link

Build # 690 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/690/ to view the results.

/** Show the view. */
void showWindow();

void setRootNode(SvnNode rootNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

javadoc?

}).catchError(new Operation<PromiseError>() {
@Override
public void apply(PromiseError error) throws OperationException {
notificationManager.notify("Error retrieving svn info", error.getMessage(), FAIL, EMERGE_MODE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the messages to locale constants

import org.eclipse.che.dto.shared.DTO;

import javax.validation.constraints.NotNull;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some info to this javadoc

@codenvy-ci
Copy link

Build # 692 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/692/ to view the results.

@@ -100,7 +100,7 @@
SVGResource status();

@Source("actions/switch.svg")
SVGResource switchLocation();
SVGResource sw();
Copy link
Contributor

Choose a reason for hiding this comment

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

sw ????

return projectUri + "/trunk";

} else if (switchView.isSwitchToBranch()) {
return projectUri + "/branches/" + (isNullOrEmpty(switchView.getSwitchToLocation()) ? "" : switchView.getSwitchToLocation());
Copy link
Contributor

Choose a reason for hiding this comment

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

move switchView.getSwitchToLocation() to var in case it used 4 times here

Copy link
Contributor

@vparfonov vparfonov Oct 12, 2016

Choose a reason for hiding this comment

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

possibly if switchView.getSwitchToLocation() == null we don't need ending "/"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it works in both cases.

this.resources = resources;
this.constants = constants;

this.ensureDebugId("svn-switch-window");
Copy link
Contributor

Choose a reason for hiding this comment

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

a lot of "this" i think :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it looks nicer :)

@codenvy-ci
Copy link

Build # 694 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/694/ to view the results.

@tolusha tolusha merged commit c3599d9 into master Oct 13, 2016
@tolusha tolusha deleted the CHE-2456 branch October 13, 2016 09:49
@codenvy-ci
Copy link

Build # 701 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/701/ to view the results.

@bmicklea bmicklea added this to the 5.0.0 milestone Jan 11, 2017
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 2, 2017
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

8 participants