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

Added further explanations about usage #5

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DandelionSprout
Copy link
Contributor

Hello, Dandelion Sprout here.

I've been successfully using this tool to create JSON versions of my Norwegian adblocking list, but it was merely the fruits at the end of my journey about learning how command lines work like, which wasn't necessarily easy peazy for someone who've been used to Windows GUIs for my entire life.

So, in order for me to help theorethical other filterlist maintainers who may not know much about command lines, I decided to suggest adding some extra notes in the Usage section to make it easier for them to properly understand how command lines work, if that's fine with you guys.

README.md Outdated
```
node abp2blocklist.js < easylist.txt > easylist.json
```

Alternately, the following command can be used in PowerShell, but it results in a larger filesize:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this result into a larger file size?

(CC: @mjethani)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't tell for sure, but I could see that while the main command resulted in a ~80kB filesize, the PowerShell-friendly command resulted in a ~150kB filesize despite having the same text contents at first glance.

@kzar
Copy link
Contributor

kzar commented May 8, 2018

Thanks for taking the time to make the pull request. I'm not sure if the command or extra instructions are necessary though. I just did a test on Windows 10 and was able to use abp2blocklist without any special steps or the get-content command. I did the following:

  1. Browsed to https://nodejs.org/en/download/ and downloaded the Windows Installer.
  2. Installed that.
  3. From the start menu opened "Node.js command prompt"
  4. Typed this: node abp2blocklist.js < easylist.txt > easylist.json

@DandelionSprout
Copy link
Contributor Author

...Ah. It never really struck me to use node.js's own command prompt system. I am going to make some changes to the pull request to reflect that. Thanks.

@DandelionSprout
Copy link
Contributor Author

DandelionSprout commented May 8, 2018

If you still find the pull request to be objectionable, I do believe that I've ticked the "Allow edits from maintainers" option, so that you can hopefully edit the pull request to your heart's content. I'm not picky.

README.md Outdated
@@ -34,13 +34,12 @@ generated file by hand.

## Usage

In Unix command lines, use:
We recommend the use of [Node.js](https://nodejs.org/en/download/)'s own command prompt, but it should also work in regular Unix command prompts (e.g. Cygwin). In either case the command is:
Copy link
Contributor

Choose a reason for hiding this comment

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

Few people use Windows to run this code, so I don't think we should assume people are running Windows in this documentation, or start off by talking about Windows specific stuff.

Copy link
Contributor Author

@DandelionSprout DandelionSprout May 8, 2018

Choose a reason for hiding this comment

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

Since some 85-90% of the world runs Windows, and that Abp2blocklist can only be run through a command prompt to the best of my knowledge, I had previously thought that they'd need extra assistance in learning about how to use command prompts in order to use this tool.

It was merely 3 months ago that I was highly confused about how command lines worked like, and I found it difficult to learn how they worked, so I figured that I should create this pull request in general to teach other list maintainers who may currently be in the situation that I was previously in.

README.md Outdated
```
node abp2blocklist.js < easylist.txt > easylist.json
```

Alternately, the following command can be used in PowerShell, but it results in a larger filesize:

We don't recommend using PowerShell, since it adds bloated metadata that increases the size of the output file, but if so needed, the following command can be used there:
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't recommend PowerShell why mention it at all or explain how to use it?

@DandelionSprout
Copy link
Contributor Author

Duly noted.

README.md Outdated
@@ -34,10 +34,13 @@ generated file by hand.

## Usage

In a Unix or Node.js command prompt, use:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can you replace this with "Create a WebKit block list output.json from the Adblock Plus filter list input.txt:"?

README.md Outdated
@@ -34,10 +34,13 @@ generated file by hand.

## Usage

In a Unix or Node.js command prompt, use:
```
node abp2blocklist.js < easylist.txt > easylist.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can you modify this example to use the names input.txt and output.json instead?

README.md Outdated
```
node abp2blocklist.js < easylist.txt > easylist.json
```

This assumes that you're trying to convert an input file that'd be called _easylist.txt_, and that you've navigated to the file's correct folder through the _cd_ command. If the input file has a different name, replace _easylist.txt_ with the file's name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can you remove this paragraph entirely?

@kzar
Copy link
Contributor

kzar commented May 8, 2018

I think there's an assumed level of knowledge about the current working directory and running Node.js with this documentation which means that we don't need to explain about the cd command for example. That said there's no harm making the example command a little easier to understand for beginners and to briefly explain what it's doing.

README.md Outdated
@@ -34,13 +34,11 @@ generated file by hand.

## Usage

In a Unix or Node.js command prompt, use:
Create a WebKit block list ```output.json``` from the Adblock Plus filter list ```input.txt```:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please could you wrap the file names in this sentence in a single backtick instead of three?

@kzar
Copy link
Contributor

kzar commented May 8, 2018

Thanks this looks good to me (LGTM) now, I'll push it for you shortly.

abpbot pushed a commit that referenced this pull request May 8, 2018
@kzar
Copy link
Contributor

kzar commented May 8, 2018

There you go: 4556506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants