-
-
Notifications
You must be signed in to change notification settings - Fork 136
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
Other: show_branch_panel and show_remote_panel #713
Conversation
So Actually, I was quite excited when I reckon we should move the development to a different branch and leave |
0dedc05
to
538ac05
Compare
What are your primary concerns regarding auto tagging @randy3k? In my mind, it seems like if we're confident enough to merge something into master, in most cases we should be confident enough to release. Even refactors are code changes thatcan introduce bugs, so a patch-version change is "correct". It also makes sense from a practical standpoint, since a release that includes 1) a refactor, and 2) an intentional behavior-change, can obscure any regressions or new bugs introduced with refactor itself. So that's my thinking, but I'm curious if there are holes in that thinking. I also want it to be useful - so if it is actively interfering, we should definitely address that somehow. |
Then it should not ask for a semver type. Not choosing one would result in a failure check.
Auto tagging is not a issue per se. I am worried about the auto-update of package control. Sublime Text users will obtain a very new piece of GitSavvy once a tag is released. It may not be idea in our case. It is not guaranteed that a hot fix in one location won't break some code in other locations. In the end, we haven't fully implemented the tests. This concern will be reduced only by enough tests.
In terms of the git flow terminology, it means the With that said, I have no objection of either models. For now, I would just create PR to Edit: Another side effect of auto-tagging is the silent update via Package Control. Without proper Package Control messages, users may be surprised GitSavvy has been updated. |
e6ada2f
to
adbdb7b
Compare
cd9c885
to
737b474
Compare
This is a lot of changes, is it possible to split it up into smaller commits? That would make it somewhat easier to review. |
Sure. |
@stoivo |
core/ui_mixins/quick_panel.py
Outdated
else: | ||
selected_branch = self.selected_branch | ||
|
||
if pre_selected_index is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always be None since it is set to None on line 260.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the comments I have. Came to d601ead and will do the rest tomorrow.
God night
core/ui_mixins/quick_panel.py
Outdated
def get_pre_selected_branch_index(self, remote): | ||
pre_selected_index = None | ||
if self.selected_branch is None: | ||
selected_branch = self.get_current_branch_name() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we set it to self.selected_branch
to avoid self.selected_branch
and selected_branch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes~
core/commands/log_graph.py
Outdated
flags=sublime.MONOSPACE_FONT, | ||
selected_index=pre_selected_index | ||
) | ||
def run_async(self, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to add **kwargs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not needed, will be removed.
core/commands/checkout.py
Outdated
self.window.show_input_panel(NEW_BRANCH_PROMPT, name, self.on_done, None, None) | ||
def run_async(self, base_branch=None): | ||
self.base_branch = base_branch | ||
self.window.show_input_panel(NEW_BRANCH_PROMPT, "", self.on_done, None, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second argument should not an empty string. When I created this it worked like if you enter an invalid branch name, for example with a space in the middle, it would open the prompt again with the name you enter prefilled.
core/commands/checkout.py
Outdated
|
||
def on_done(self, branch_name): | ||
pattern = r"^(?!\.|.*\.\..*|.*@.*|\/)[a-zA-Z0-9\-\_\/\.\u263a-\U0001f645]+(?<!\.lock)(?<!\/)(?<!\.)$" | ||
match = re.match(pattern, branch_name) | ||
if not match: | ||
sublime.error_message("`{}` is a invalid branch name.\nRead more on $(man git-check-ref-format)".format(branch_name)) | ||
sublime.error_message(NEW_BRANCH_INVALID.format(branch_name)) | ||
sublime.set_timeout_async(self.run_async(branch_name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the recursion if you enter an invalid branch name.
core/commands/checkout.py
Outdated
v.run_command("select_all") | ||
|
||
def on_enter_local_name(self, branch_name): | ||
pattern = r"^(?!\.|.*\.\..*|.*@.*|\/)[a-zA-Z0-9\-\_\/\.\u263a-\U0001f645]+(?<!\.lock)(?<!\/)(?<!\.)$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be moved into a constant to remove duplication.
self.on_enter_local_name, | ||
None, | ||
None) | ||
v.run_command("select_all") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done for the the above one too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, now I am at commit 591edb0
core/commands/fetch.py
Outdated
@@ -19,39 +17,22 @@ def run(self, remote=None): | |||
if remote: | |||
return self.do_fetch(remote) | |||
|
|||
self.remotes = list(self.get_remotes().keys()) | |||
show_remote_panel(self.on_remote_selection, show_all=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename show_all
to show_option_all
? this way is more self-explanatory. Not a big deal but show_all is quite ambiguous.
core/commands/commit_compare.py
Outdated
@@ -146,35 +146,22 @@ def run(self, base_commit=None, target_commit=None, file_path=None): | |||
self._target_commit = target_commit | |||
sublime.set_timeout_async(self.run_async) | |||
|
|||
def run_async(self): | |||
self.all_branches = [b.name_with_remote for b in self.get_branches()] | |||
def run_async(self, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must be copied from other place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After these comments you can squarsh all fixup commit into where they belong.
Then
pre_selected_index = 0 | ||
|
||
self.view.window().show_quick_panel( | ||
self.remote_keys, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line 6322960#diff-7307dc85c05c5908333cc4d57b32b2d8R34 unused variable
6322960#diff-7307dc85c05c5908333cc4d57b32b2d8R91
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
self.git( | ||
"checkout", "-b", | ||
branch_name, | ||
self.base_branch if self.base_branch else None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to pass a starting point?
I don't think we need to pass a starting point, if so self.base_branch is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean base_branch
? It will be used in dashboard to create new branch for other branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the command would look like
git checkout -b <branch_nane> <starting point>
Should we pass the starting point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is useful to specific the starting point in some cases. The default value of base_branch
is None. It would not produce any side effects in the normal use case.
Other: remove the redundant if condition Other: remove the unneccessary use of selected_branch
Other: remove unneeded keyword arguments
Other: use the invalid branch name as default
Other: more self explanatory argument show_option_all
Other: remove the unneccessary keyword argument
Other: remove the unneccessary argument
all rebased. Thanks @stoivo to review it. |
def run_async(self, name=""): | ||
self.window.show_input_panel(NEW_BRANCH_PROMPT, name, self.on_done, None, None) | ||
def run_async(self, base_branch=None): | ||
self.base_branch = base_branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are setting it here so it does not work if you change the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am lost. If base_branch
is None, it is just like previously. Else if base_branch
is not None, it will checkout a new branch from base_branch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I thought I broke it. But when I test it now I works perfectly
def run_async(self, name=""): | ||
self.window.show_input_panel(NEW_BRANCH_PROMPT, name, self.on_done, None, None) | ||
def run_async(self, base_branch=None): | ||
self.base_branch = base_branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I thought I broke it. But when I test it now I works perfectly
close #546
It is a major refactor of the branch and remote panels. The two related functions are show_remote_panel and show_branch_panel.
The git flow related code is kept untouched as they are written in a different style with the other code.
I tried very hard to make sure it doesn't break any functionality, though not 100% sure.
The maintainers of this repo require that all pull request submitters agree and adhere to the following:
(required)
The maintainers of this repository require you to select the semantic version type that
the changes in this pull request represent. Please select one of the following: