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

Option to ignore whitespace #28

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

GiovanH
Copy link

@GiovanH GiovanH commented Nov 7, 2021

Whitespace is left in place. Only lines with content are deduplicated.

The ignore list could potentially be extended to be configurable.

@ilyakam
Copy link
Owner

ilyakam commented Nov 9, 2021

Hello @GiovanH and thank you for the contribution! Apologies for not getting back to you about this sooner.

There are a few things that I'd like to address with regards to this PR and to hear your thoughts about them.

  1. What problem are you trying to solve?
    As seen in the Order of Operations section of the CONTRIBUTING.md document, I'd prefer it if you started by describing the issue. Based on the code and the description above, it sounds like you don't wish to consider empty lines as duplicates of one another. If so, may I ask why? Multiple lines with no characters are technically duplicates of one another, are they not? Moreover, I'm in the process of moving over to VS Code and another plugin called Text Power Tools that was developed independently from this one (to the best of my knowledge) is acting in the exact same way:
ST3 VSC
RDL - PR28 - Current VS Code - TPT - RDL
  1. You mentioned that "[t]he ignore list could potentially be extended to be configurable." I agree! If you can convince me of the use case in the previous question, then there's already a pattern that allows you to customize the settings. Please take a look at the *.sublime-settings files in my other ST3 plugins, NumberFormatter and ParentalControl. The gist is this code. In my opinion, that's much better than having a hardcoded regex that can only be changed by releasing an update to this plugin.

  2. Thanks for including a separate "Basic hygiene" commit (49e4a0d)! Having reviewed this section of the PEP8 guide, it looks like I need to update the following .editorconfig rule:

    indent_size = 2
    I'm going to open a new issue for that. EDIT: Adhere to the PEP8 Style Guide #29

@GiovanH GiovanH marked this pull request as draft November 9, 2021 01:44
@GiovanH
Copy link
Author

GiovanH commented Nov 9, 2021

My apologies, this was supposed to still be a draft PR. Yes, I agree with making the ignorelist a configurable setting, and it should probably be empty by default. This was one of those "I wrote it, so let me make a note before I forget" situations.

@GiovanH GiovanH changed the title Ignore whitespace Option to ignore whitespace Nov 9, 2021
@GiovanH GiovanH marked this pull request as ready for review November 11, 2021 00:02
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.

2 participants