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

Added custom name for favorites item #601

Merged
merged 10 commits into from Sep 22, 2016

Conversation

Projects
None yet
4 participants
@IgorGalarraga

IgorGalarraga commented Sep 16, 2016

  • Refs: #599
  • Custom name for favorites item.
  • Recent item name is now full path.
  • Very initial implementation. My first contact with Python.
  • TODO:
  • Old settings file (~/.config/git-cola/settings) compatible.
  • Unify 2 prompt windows when add favorite on one window.
  • Traductions for new strings.
  • Add rename line on contextual menu.
  • Extend renaming to recent repo list.
  • Pass Travis C
Added custom name for favorites item
Recent item name is now full path.
@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Sep 17, 2016

Member

Thanks for this first version. It looks like we're on the right track. The backwards-compatibility thing is probably essential to iron out before rolling this in. I'll see if I can lend a hand in the coming weeks.

Please run the test suite via make test. You may have noticed that the Travis CI build failed because the tests were broken by these changes, so we'll have to get that in order too.

Let me know if you have any questions. I'll do a thorough review of this first version over the weekend.

Member

davvid commented Sep 17, 2016

Thanks for this first version. It looks like we're on the right track. The backwards-compatibility thing is probably essential to iron out before rolling this in. I'll see if I can lend a hand in the coming weeks.

Please run the test suite via make test. You may have noticed that the Travis CI build failed because the tests were broken by these changes, so we'll have to get that in order too.

Let me know if you have any questions. I'll do a thorough review of this first version over the weekend.

@Lin-Buo-Ren

This comment has been minimized.

Show comment
Hide comment
@Lin-Buo-Ren

Lin-Buo-Ren Sep 17, 2016

Contributor

I haven't tried the patch, but I wonder if it's sane to rather use the <repo>/.git/name <repo>/.git/description conventional(?) file for repo description instead.

what should be in the git description file? - Stack Overflow

Contributor

Lin-Buo-Ren commented Sep 17, 2016

I haven't tried the patch, but I wonder if it's sane to rather use the <repo>/.git/name <repo>/.git/description conventional(?) file for repo description instead.

what should be in the git description file? - Stack Overflow

@IgorGalarraga

This comment has been minimized.

Show comment
Hide comment
@IgorGalarraga

IgorGalarraga Sep 17, 2016

@Lin-Buo-Ren Is the first time that I ear about .git/description file for GitWeb application. Thanks!.
After evaluate this a bit, I think that the best way to add this file usage could be:

  • Mantain repo path and name on git-cola settings. Like now.
  • On Favorite add action, check this file: if it has a name, propouse this name. If it is default (as they are in all my repos! ;) ) propouse repo base name like now.
  • On settings create a new check box to select or not this file sincronization with git-cola renaming system.

IgorGalarraga commented Sep 17, 2016

@Lin-Buo-Ren Is the first time that I ear about .git/description file for GitWeb application. Thanks!.
After evaluate this a bit, I think that the best way to add this file usage could be:

  • Mantain repo path and name on git-cola settings. Like now.
  • On Favorite add action, check this file: if it has a name, propouse this name. If it is default (as they are in all my repos! ;) ) propouse repo base name like now.
  • On settings create a new check box to select or not this file sincronization with git-cola renaming system.
@IgorGalarraga

This comment has been minimized.

Show comment
Hide comment
@IgorGalarraga

IgorGalarraga Sep 17, 2016

@davvid I know that the backwards-compatibility is very important, but it seem difficult for me and for now with my limited knowledge on Python, anyway, I will try to do it.
After pull-request I check Travis CI errors, I will try to pass them: some functions has changed their number of parameters.

IgorGalarraga commented Sep 17, 2016

@davvid I know that the backwards-compatibility is very important, but it seem difficult for me and for now with my limited knowledge on Python, anyway, I will try to do it.
After pull-request I check Travis CI errors, I will try to pass them: some functions has changed their number of parameters.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Sep 18, 2016

Member

On this line and various others in this PR there's a small style nit. Add a single space after commas in expressions.

Member

davvid commented on test/settings_test.py in 9e88319 Sep 18, 2016

On this line and various others in this PR there's a small style nit. Add a single space after commas in expressions.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Sep 18, 2016

Member

Drop the space after {, before :, and before } above. e.g.

bookmark = {'path': '/tmp/this/does/not/exist', 'name': 'notexist'}
Member

davvid commented on test/settings_test.py in 9e88319 Sep 18, 2016

Drop the space after {, before :, and before } above. e.g.

bookmark = {'path': '/tmp/this/does/not/exist', 'name': 'notexist'}
@davvid

Getting really close. After we address these last notes this looks just about ready to go. Thanks again.

Show outdated Hide outdated cola/settings.py
if not self.verify(bookmark):
missing_bookmarks.append(bookmark)
else:
update_bookmarks.append(new_bookmark)

This comment has been minimized.

@davvid

davvid Sep 18, 2016

Member

It looks like we're dealing with the old bookmarks format in self.remove_missing() which is probably a bit too late to upgrade the data.

Instead of doing a try/except we can check if isinstance(bookmark, dict): and detect the new format, since the old format will be a string.

A better place to fix this would be to add a separate upgrade() method and call it from load() before self.remove_missing() is called. Try something like this... (if I'm understanding the new format correctly)

def upgrade(self):
    """Upgrade the settings format from v1 to v2"""
    if self.bookmarks and not isinstance(self.bookmarks[0], dict):
        self.bookmarks = [{'name': os.path.basename(i), 'path': i} for i in self.bookmarks]

this'll allow remove_bookmarks() to only deal with the new format.

@davvid

davvid Sep 18, 2016

Member

It looks like we're dealing with the old bookmarks format in self.remove_missing() which is probably a bit too late to upgrade the data.

Instead of doing a try/except we can check if isinstance(bookmark, dict): and detect the new format, since the old format will be a string.

A better place to fix this would be to add a separate upgrade() method and call it from load() before self.remove_missing() is called. Try something like this... (if I'm understanding the new format correctly)

def upgrade(self):
    """Upgrade the settings format from v1 to v2"""
    if self.bookmarks and not isinstance(self.bookmarks[0], dict):
        self.bookmarks = [{'name': os.path.basename(i), 'path': i} for i in self.bookmarks]

this'll allow remove_bookmarks() to only deal with the new format.

This comment has been minimized.

@IgorGalarraga

IgorGalarraga Sep 18, 2016

Two bookmarks version are a list (of different components). self.remove_missing() works well on both settings file versions (on my tests), because it deals with whole unit on list not with content, after this parse, the bookmark will be updated.
Yes, try/except not is more elegant, the use of if isinstance(bookmark, dict): is the correct choice for this code.
In my first try, I wanted do a first upgrade parse, but I did not know to find the solution in Phyton... thanks for your example.

@IgorGalarraga

IgorGalarraga Sep 18, 2016

Two bookmarks version are a list (of different components). self.remove_missing() works well on both settings file versions (on my tests), because it deals with whole unit on list not with content, after this parse, the bookmark will be updated.
Yes, try/except not is more elegant, the use of if isinstance(bookmark, dict): is the correct choice for this code.
In my first try, I wanted do a first upgrade parse, but I did not know to find the solution in Phyton... thanks for your example.

This comment has been minimized.

@IgorGalarraga

IgorGalarraga Sep 18, 2016

My comment:

Two bookmarks version are a list (of different components). self.remove_missing() works well on both settings file versions (on my tests), because it deals with whole unit on list not with content, after this parse, the bookmark will be updated.

was an explanation of behavior of self.bookmarks.remove(bookmark) into def remove_missing(self), not for self.remove_missing()...

@IgorGalarraga

IgorGalarraga Sep 18, 2016

My comment:

Two bookmarks version are a list (of different components). self.remove_missing() works well on both settings file versions (on my tests), because it deals with whole unit on list not with content, after this parse, the bookmark will be updated.

was an explanation of behavior of self.bookmarks.remove(bookmark) into def remove_missing(self), not for self.remove_missing()...

Show outdated Hide outdated cola/settings.py
for bookmark in missing_bookmarks:
try:
self.bookmarks.remove(bookmark)
except:
pass
"""In case of old settings version, clear old bookmars list and fill with new list"""

This comment has been minimized.

@davvid

davvid Sep 18, 2016

Member

A python idiom is that we really only ever use """ for docstrings. For in-line comments just use # .... I guess that probably wasn't clear because the codebase is probably a little terse/under-commented ;-)

@davvid

davvid Sep 18, 2016

Member

A python idiom is that we really only ever use """ for docstrings. For in-line comments just use # .... I guess that probably wasn't clear because the codebase is probably a little terse/under-commented ;-)

This comment has been minimized.

@IgorGalarraga

IgorGalarraga Sep 18, 2016

Thanks for the clarification.

@IgorGalarraga

IgorGalarraga Sep 18, 2016

Thanks for the clarification.

@IgorGalarraga

This comment has been minimized.

Show comment
Hide comment
@IgorGalarraga

IgorGalarraga Sep 18, 2016

When add def upgrade(self): I have 2 problems:

  • I can't directly update self.bookmarks like your example. I must use other list and use self.bookmarks.remove and self.bookmarks.append. BUT:
  • It seems that changes are overwritten (with file contents in old version) again in remove_missing

For better explanation, I attach a patch with working (with problems above) method.
0001-Update-bookmarks-method-but-has-problems.patch.txt

Therefore I have changed try/except using isinstance.

Also code style has been updated.

I am not able to make a prompt window with two text boxes... any help (url, reference, example)?

Thanks!

IgorGalarraga commented Sep 18, 2016

When add def upgrade(self): I have 2 problems:

  • I can't directly update self.bookmarks like your example. I must use other list and use self.bookmarks.remove and self.bookmarks.append. BUT:
  • It seems that changes are overwritten (with file contents in old version) again in remove_missing

For better explanation, I attach a patch with working (with problems above) method.
0001-Update-bookmarks-method-but-has-problems.patch.txt

Therefore I have changed try/except using isinstance.

Also code style has been updated.

I am not able to make a prompt window with two text boxes... any help (url, reference, example)?

Thanks!

davvid added a commit to davvid/git-cola that referenced this pull request Sep 22, 2016

settings: upgrade bookmarks immediately after loading
Simplify the code by upgrading bookmarks immediately instead of making
the remove_missing() function deal with the format differences.

Related-to: #599
Related-to: #601
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Sep 22, 2016

settings: small style tweaks
Related-to: #601
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Sep 22, 2016

startup: remove repeated code
Make the code more closely match the original version by homogenizing
the data rather than having two code paths.

Related-to: #601
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Sep 22, 2016

bookmarks: wrap long line
Related-to: #601
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Sep 22, 2016

bookmarks: use settings.rename_bookmark()
Related-to: #601
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit to davvid/git-cola that referenced this pull request Sep 22, 2016

Merge branch 'rename-favorites'
* rename-favorites:
  doc: update v2.9 release notes draft
  bookmarks: use settings.rename_bookmark()
  settings: add rename_bookmark() method
  bookmarks: wrap long line
  startup: remove repeated code
  settings: small style tweaks
  settings: upgrade bookmarks immediately after loading
  Added favorites list item renaming functionality
  Bugfix: default repo now working again on favorites.
  Bugfix: startup problem on launching git-cola in no git repo path
  Code style
  Using isinstance(bookmark,dict) instead of use try/except
  Backwards-compatibility with old settings file
  Automatically updates old settins to new bookmarks
  make test passed for last modification.
  Added custom name for favorites item

Closes #599
Closes #601
Signed-off-by: David Aguilar <davvid@gmail.com>

@davvid davvid merged commit ed45534 into git-cola:master Sep 22, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment