Adding protection string options for Issue #9

[nickpharrison]

No description provided.

[j4w8n]

@nickpharrison did you get notification of my review comments? Seems like I've had issues with that before, so wanted to follow-up.

[nickpharrison]

@nickpharrison did you get notification of my review comments? Seems like I've had issues with that before, so wanted to follow-up.

@j4w8n Unfortunately I didn't get any email. I have however just made one small fix to my original commit.

[j4w8n]

Ok, thanks @nickpharrison. Do you at least see my review comments?

[nickpharrison]

@j4w8n I'm afraid not, they're not showing up in the usual place. Could you send a link? Maybe I'm looking in the wrong place.

[j4w8n]

@nickpharrison, after watching a youtube video on it (I'm such a noob lol), I realized I did not officially finish my review. Should be viewable now.

Thanks!

[j4w8n]

@nickpharrison I'm close to merging this.

I've been doing a lot of thinking on verbiage around "ignore", "protect", etc. One thing that led me to this is that one of the configuration options is ignore and now we're gonna have a related one called protection_string. The issue is that looking at those two names, you can't infer that they're related, and I'd like people to be able to. And I'd like to resolve this in a weird way.

Let me know if you think I'm making a mistake here:

Can you change that option to preservative? In a future PR, I'll deprecate ignore and add preserve. I racked my brain trying to come up something that aligns and works - ignore/ignore_with, freeze/freeze_with, etc. Preserve seems to be the best word I've thought of so far. I'm not a superfan of preservative but it fits.

Also, no matter what the name is, I think we can move your string into the default config of the constants.js file. This is because when we validate the config, the user config is merged with the default.

And lastly, instead of adding multiple arguments to ignoreElement, how about we just pass in the full config? Actually, I'll take care of this in another PR, because it's gonna require additional work and I'd like to get your changes merged.

[nickpharrison]

@j4w8n completely agreed the names should be aligned, protection_string definitely isn't ideal in retrospect.

Personally I prefer the _with syntax, but at the end of the day as long as it is documented any name will be fine.

If it was me I'd lean towards protect ignore and protect_with ignore_with without changing the naming convention away from the one currently used. But again, as long as it's documented and backward compatible it will be fine.

Let me know your thoughts and I'm happy to make the changes.

[j4w8n]

@j4w8n completely agreed the names should be aligned, protection_string definitely isn't ideal in retrospect.

Personally I prefer the _with syntax, but at the end of the day as long as it is documented any name will be fine.

If it was me I'd lean towards protect ignore and protect_with ignore_with without changing the naming convention away from the one currently used. But again, as long as it's documented and backward compatible it will be fine.

Let me know your thoughts and I'm happy to make the changes.

Yeah, I thought of keeping ignore when I was mulling this over with my wife (not technical at all), so that there wouldn't be any deprecation. Let's roll with what you said. Thanks for your feedback!

[j4w8n]

Anything else you need from my @nickpharrison?

[nickpharrison]

@j4w8n I've just made the changes we talked about and added a test case. Take a look and let me know what you think.

[j4w8n]

@nickpharrison looks great, thanks!