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

Add DuplExpellio package to Package Control Channel #9012

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JohhannasReyn
Copy link
Contributor

  • I'm the package's author and/or maintainer.
  • I have have read the docs.
  • I have tagged a release with a semver 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 to exclude files from the package: images, test files, sublime-project/workspace.

My package is unique.

There are no packages like it in Package Control.

My package is similar to ... However it should still be added because ...

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: DuplExpellio

Packages added:
  - DuplExpellio

Processing package "DuplExpellio"
  - All checks passed

@braver
Copy link
Collaborator

braver commented Nov 30, 2024

Thanks for your submission. Some remarks:

  • If I understand correctly, the behavior is toggled via a setting. Wouldn't it be easier if there were two commands: one to select all duplicates, one to delete them?
  • I also don't fully understand the use case yet. What is considered a duplicate? A character, a word, a line? The threshold setting is also a bit mystifying to me.

@JohhannasReyn
Copy link
Contributor Author

By duplicate I mean a duplicate entry in a delimited file, whether it's delimited by a new line (targeting duplicate lines), by comma, by semi-colon, or by a space (targeting duplicate words) the delimiter can be specified by the user in the settings as well as how many of them it takes to qualify as a delimiter so that the plugin can make its way down the list until a delimiting string or character satisfies the criterium (i.e. to not accidentally use commas for a semi-colon separated file.)

I like your suggestion of making this into two separate commands, I will implement this now and create a new pull request, if you could reject this one, assuming it's not too late, as I have encountered an error with my latest release (I apologize) I noticed just in the last hour that the plugin is failing to work whilst in the zipped .sublime-package form, and is only functional as an unzipped package. I know where I messed up, and will have it corrected and should have it followed with a new pull request in the next couple hours. Again, I apologize for my oversight. Please don't fire me, haha.

@braver
Copy link
Collaborator

braver commented Nov 30, 2024

Please don't fire me, haha.

🤣

Sure, no worries.

Thanks for the explanation, I can definitely see use cases for that!

The change in behavior when your code gets packaged is one we all run into every now and then. Let me know, in this PR, when you’ve completed the changes you wanted to make. We’ll merge ASAP then.

@braver
Copy link
Collaborator

braver commented Dec 13, 2024

I'm not sure your use of package_loaded() is useful. Each time you call settings.get() you're already correctly passing defaults, so there is no need to set those defaults on package load. In fact, since you provide default settings via the settings file, even passing those defaults again in the getter isn't even necessary either.

Printing anything at package load is then also not useful and just adds noise to the console. In fact, printing anything, e.g. on calling a command, adds noise and should be prevented during normal usage. You could add a debug mode, or remember to remove those lines before you tag a release.

@braver
Copy link
Collaborator

braver commented Dec 13, 2024

By duplicate I mean a duplicate entry in a delimited file

I also think you might want to expand on that a bit in the readme, perhaps add some examples. Because if I open any random file and then run your command, I still don't fully understand what to expect.

@braver braver added the mergeable The PR is in a mergeable state but awaiting some final comments/acknowledgement from either side label Dec 13, 2024
@JohhannasReyn
Copy link
Contributor Author

JohhannasReyn commented Dec 13, 2024 via email

@braver
Copy link
Collaborator

braver commented Dec 14, 2024

Package Control and our tools for this review only see your latest tag. So if you’re hoping for a code change to help pass the automated test, you do need to create a new tag first (and then request a new review from the bot).

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants