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

Diffable package added. #8816

Merged
merged 4 commits into from
Oct 11, 2023
Merged

Diffable package added. #8816

merged 4 commits into from
Oct 11, 2023

Conversation

yaroslavyaroslav
Copy link
Contributor

@yaroslavyaroslav yaroslavyaroslav commented Sep 12, 2023

  • I'm the package's author and/or maintainer.
  • I have have read [the docs][1].
  • I have tagged a release with a [semver][2] version number.
  • My package repo has a description and a README describing what it's for and how to use it.
  • My package doesn't add context menu entries. *
  • My package doesn't add key bindings. **
  • Any commands are available via the command palette.
  • Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.
  • If my package is a syntax it doesn't also add a color scheme. ***
  • If my package is a syntax it is named after the language it supports (without suffixes like "syntax" or "highlighting").
  • I use [.gitattributes][3] to exclude files from the package: images, test files, sublime-project/workspace.

My package is a wrapper around sublime built in incremental diff tool, plus in addition it provides an option to pass the content to Kaleidoscope diff app.

My package is similar to Diffy. However it should still be added because diffy is quite deprecated for a years, I've even tried to contribute there, and my PR even got accepted, yet it's still latest version in the channel is from 2015 year. Also it has quite ugly UI and inaccurate and slow diff also.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: ERROR

Repo link: Diffable
Results help

Packages added:
  - Diffable

Processing package "Diffable"
  - ERROR: The binding ['super+k', 'super+d'] unconditionally overrides a default binding
    - File: Default (OSX).sublime-keymap
  - ERROR: The binding ['super+k', 'super+c'] unconditionally overrides a default binding
    - File: Default (OSX).sublime-keymap
  - ERROR: The binding ['ctrl+k', 'ctrl+d'] unconditionally overrides a default binding
    - File: Default (Windows).sublime-keymap
  - ERROR: The binding ['ctrl+k', 'ctrl+c'] unconditionally overrides a default binding
    - File: Default (Windows).sublime-keymap
  - ERROR: The binding ['ctrl+k', 'ctrl+d'] unconditionally overrides a default binding
    - File: Default (Linux).sublime-keymap
  - ERROR: The binding ['ctrl+k', 'ctrl+c'] unconditionally overrides a default binding
    - File: Default (Linux).sublime-keymap

@yaroslavyaroslav
Copy link
Contributor Author

Those conflicted bindings are dismissed now. Could some one please rerun these tests?

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: Diffable

Packages added:
  - Diffable

Processing package "Diffable"
  - All checks passed

@yaroslavyaroslav
Copy link
Contributor Author

@braver hey, just wondering is it something wrong with this package or it's just waiting its turn to be properly reviewed?

@braver
Copy link
Collaborator

braver commented Sep 21, 2023

This is a community project, we'll get to it when we get to it. It usually takes a few weeks for new submissions to float to the top of the stack.

Couple of things:

  • First off: looks good 👍🏻
  • I think it would be a good idea to replace the existing Diffy package. It's well and truly dead it seems. Is yours a drop-in replacement for existing users?
  • Please add a setting or something so that users can easily disable the context menu. It takes up prime real estate and it's not necessarily the easiest way of interacting with it.
  • You specifically implement the Kaleidoscope app. It looks like it takes pretty standard arguments (left file + right file), and a lot of other diffing apps will work the same way. See also SidebarTools. In fact, might make sense to be able to select 2 files in the sidebar instead of specifically 2 views.
  • In your Main Menu, for those standard nodes like "Preferences", just provide the id, not the caption.

@yaroslavyaroslav
Copy link
Contributor Author

yaroslavyaroslav commented Sep 21, 2023

Thanks for kinds words.

- [+] I think it would be a good idea to replace the existing Diffy package. It's well and truly dead it seems. Is yours a drop-in replacement for existing users?
- [-] Please add a setting or something so that users can easily disable the context menu. It takes up prime real estate and it's not necessarily the easiest way of interacting with it.
- [-] You specifically implement the Kaleidoscope app. It looks like it takes pretty standard arguments (left file + right file), and a lot of other diffing apps will work the same way. See also [SidebarTools](https://github.com/braver/SideBarTools/blob/master/SideBar.py#L53). In fact, might make sense to be able to select 2 files in the sidebar instead of specifically 2 views.
- [x] In your Main Menu, for those standard nodes like "Preferences", just provide the id, not the caption.
  • I'm quite confused. As long as I'm aware there's now way to enable context menu block (e.g. DIffable), GPT-4 haven't help me here also. I believe the better way would be just to drop it completely.

  • I'm now sure about your third point. If you are talking about to generalise third party tool comparison here? I'm up for that, but just haven't tested whether the given python code would fit other diff tools.

  • And about the first one: What practically outcome from that point? Do you suggest to rename the whole plugin to Diffy don't you? If so again I'm confused coz I believe this would lead to git repo renaming, isn't it?

@braver
Copy link
Collaborator

braver commented Sep 24, 2023

As long as I'm aware there's now way to enable context menu block

  • I would get rid of the separator. Not every group of commands needs it's own delineated block, that just takes up space.
  • I kind of assume that the "Diffable (2 Columns)" parent item disappears if all commands in it are unavailable. You could make an intermediate command for the context menu items, and set the is_visible for that command to return false if the context menu is disabled via a setting.

If you are talking about to generalise third party tool comparison here

More as a suggestion for future development. Kaleidoscope is very niche, and lots of other apps can be launched in a similar manner.

Do you suggest to rename the whole plugin to Diffy don't you?

Package control references packages by name. If your package is mostly a drop in replacement for another abandoned package, you could change the details of that package to point to your repo. Or we remove the existing package and add its name to your package via previous_names. Some examples: https://github.com/wbond/package_control_channel/pulls?q=is%3Apr+label%3Atakeover+is%3Aclosed

@braver braver added the mergeable The PR is in a mergeable state but awaiting some final comments/acknowledgement from either side label Sep 24, 2023
@yaroslavyaroslav
Copy link
Contributor Author

yaroslavyaroslav commented Sep 24, 2023

  • I kind of assume that the "Diffable (2 Columns)" parent item disappears if all commands in it are unavailable.

Actually I assumed it either, but it appears that it's not. To be honest I'm completely confused with this very feature at a whole. (1) menu doesn't disappear even if it's empty (see screenshot, Diffable menu are hovered). (2) it failed to work well if I'm following examples that you've provided above: both function is_visible and is_available have different attribute signatures in the current ST version, so I've to use a workaround for those.

Anyway, I consider your issue with menu as a fair one, so I implemented it in a way as I was able it to settle.

Package control references packages by name.

Done, should I do something else within this repo by myself to evaporate Diffy from a package control search?

UPD: Diffy entity deleted.

Screenshot 2023-09-24 at 13 51 21

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Packages modified:
  - Diffable

@braver
Copy link
Collaborator

braver commented Sep 25, 2023

should I do something else within this repo by myself to evaporate Diffy from a package control search

Yes, in fact, the existing entry for Diffy should be removed.

@yaroslavyaroslav
Copy link
Contributor Author

@braver Yep, it's already done, CI was failing otherwise.

@braver
Copy link
Collaborator

braver commented Sep 26, 2023

Alright, looking good. As a matter of protocol I would like to give @zsong 2 weeks to object to the replacement and come up with an alternative approach. If nothing comes from that, we'll merge this PR.

@braver braver added the takeover Package maintainership is changing label Sep 26, 2023
@yaroslavyaroslav
Copy link
Contributor Author

@braver Hi, seems like the time has come.

@braver
Copy link
Collaborator

braver commented Oct 10, 2023

Alright, I would say that all that's missing now is a 2.0 upgrade message (for existing users) explaining the change of name and ownership of the package and any changes that might be relevant for them.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Packages modified:
  - Diffable

@yaroslavyaroslav
Copy link
Contributor Author

@braver
Copy link
Collaborator

braver commented Oct 11, 2023

I think you need to put the full path to the 2.0.0 message in the json (ie. unlike the readme it's not in the root dir).

@yaroslavyaroslav
Copy link
Contributor Author

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Packages modified:
  - Diffable

@yaroslavyaroslav
Copy link
Contributor Author

yaroslavyaroslav commented Oct 11, 2023

Apparently revealed that there's an option to add donation page to a package, therefore added one.

@braver braver merged commit 047a325 into wbond:master Oct 11, 2023
2 checks passed
@yaroslavyaroslav yaroslavyaroslav deleted the diffable branch October 11, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback provided mergeable The PR is in a mergeable state but awaiting some final comments/acknowledgement from either side takeover Package maintainership is changing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants