Redundancy in `edits.reason` field

For example, if you do Ctrl-F on App.consts.reasons.grammar, there's too many successive fields in all having the same reason.

How about you spin off the same reason into a different array? (same for other App.consts.reasons)

For example, this is how it currently looks like:

App.edits = {
    ...
    apple: {
        expr: /\bapple\b/g,
        replacement: "Apple",
        reason: App.consts.reasons.trademark
     },
     iphone: {
        expr: /\biph?one?\b/gi,
        replacement: "iPhone",
        reason: App.consts.reasons.trademark
    ...
};

Here's how I think it would work better:

App.edits = {};
App.edits.trademark = [ // as a nested array
    ...
    [/\bapple\b/g, "Apple"],
    [/\biph?one?\b/gi, "iPhone"]
    ...
];

I hope my suggested idea is clear. I would be happy to do a PR for the same.

With respect to the specific organization you propose, I'd decline.

Having each replacement be an Object identified as a key/value pair increases flexibility and is beneficial for debugging. Currently, there are a variety of additional optional properties available for each rule. Using an Array to represent the replacement makes it significantly less convenient to use these, or create others, when needed.

I'll add some documentation to the code as to what properties are available for each edit rule Object. This will be in the next version I push.

Using Objects in key/value pairs does have some drawbacks, but, on balance, I'd rather live with those. That doesn't mean that we can't do something to make things more convenient.

What issue(s) are you attempting to solve by this proposed change?

It's not clear to me exactly what your goal(s) is/are for this change. All you've said is "there's too many successive fields in all having the same reason". You haven't described why this is a problem, or what issue you're really wanting to solve. Currently, what you've said boils down to "I don't like it." There may be real and good reasons to make changes here, but you haven't explained what those are.

While I'm open to making changes, without knowing what your goals are, it's impossible to evaluate if what you're proposing solves the issue(s) you're trying to address, if we agree that those issues are something which should be addressed, and/or if those issues are already addressed in some other manner.

Is it to determine which edit rule made changes?

If the issue you are attempting to address is to determine exactly which edit rules produced changes, that's not intended to be determined using the reason. The reason is intended to be human readable, for use in the edit summary, merely to identify the type of change which was made.

For determining which edit rule made the change, that's what the edit rule's key is for. Showing which rules made a change is something that should, at most, be logged to the console (easily done in 1 line). It's debatable if we should always do so, but I'd lean towards doing so, as it allows people to go to the console immediately when they experience a problem and see which rules made changes, without the need to make changes to the script just to determine which rule made a change.

I'll add logging of this information to the next version (already existed in my version for debug; needed some cleanup; done). This will be in the next version I push, along with some additional cleanup.

Is it just searching?

You're searching on the reason that's displayed in the edit summary. That information isn't intended to provide an accurate way to determine which edit rule was applied. Currently, the facility to do that is to change App.globals.showRules to true. However, I agree that having the edit rules which actually made changes more immediately available in the console would be beneficial (see change mentioned above).

Is it ease of editing?

Having most of the edit rules represented as a single line of code would, probably, be easier for editing rules. This could be accomplished in a variety of ways. The one that comes most readily to mind is to have constructor functions (i.e. backward compatible JavaScript classes) for the different types of edit rules. Doing so could make the syntax for defining an edit rule look something like:

apple: new TrademarkRule(/\bapple\b/g, "Apple"),
iphone: new TrademarkRule(/\biph?one?\b/gi, "iPhone"),
functionWithMoreProperties: new TrademarkRule(/some(th)ing/, function(match, p1, offset, fullString) {
    //fullString will usually be only the matched text, but may be the entire post (with placeholders for code/links/etc.)
    ...
}, {
    debug: true,
}),

If done this way, doing so would take marginally more time at load, so it would be better to wait to construct the Objects until the user clicks on the Magic Editor button.

It's also not clear to me that the benefit to editing (an infrequent activity) is worth the effort of making a change here.

Is it to save characters?

The representation you've proposed does save considerable characters, but that's really a non-issue for a userscript.

Something else?

What issue(s) are you attempting to solve by this proposed change?

Sorry, you're right. I wasn't as elaborate as I should've been. I didn't mean to come out as "I don't like the design", so probably the following explanation is a bit clearer. The main issue is redundancy and segregation. Look at the line - reason: App.consts.reasons.trademark. It is repeated a dozen times through the code. I think it impacts the following two things

extensibility - what if maybe I make a change in the App.consts structure. Then I would need to make that change in every single rule. I know Find and Replace tools make it easy, but then it is avoidable.
segregation - this is the more important one. For example, having all trademark related rules in one place together gives a clear picture of how many are there, and overall benefits the organization of the rules. For example, in case you batch-operate on all trademark related rules (disable all of them temporarily), you cannot do that with the current setup. But you can do so if you keep all trademark related rules in one place.

Now, this was just for the .trademark rule, there's also rules on .grammar, .noise, etc. So, the impact of this problem is much larger.

I understand that "if it it ain't broke, don't fix it". But, my main concern was about segregation and keeping things DRY. Also, I am okay if you're currently not happy in proceeding with the changes, I just wished to put this message out there in case you may consider it.

Also, thanks for taking the time to write up in detail the different ideas there! I believe I concur with you on all of those points, however, the point I have detailed now is probably separate from all of them.

Hi, did you manage to push an update for the "This will be in the next version I push, along with some additional cleanup."?

The reason is that I wish to port the Magic Editor for Chem.SE. I had already begun writing a short userscript myself, but I really like how Magic Editor display a revision history before and after the changes, as well as fixes the punctuation and several other important details.

For Chem.SE, I need to add - for example - a regex to lowercase all 118 element names ("Uranium" => "uranium").

Given that you said you are optimizing parts of Magic Editor, should I wait for the changes, or start extending for Chem.SE right away? (because I don't know what types of changes you're making, I'm not sure if they'll be backwards compatible or not, hence I am asking before starting work on this)

I had basically paused making major changes based on an assumption that you were going to make the changes you said you wanted to do in issue #134. (note: please have those changes in a separate PR from the ones you've mentioned in your most recent comment here).

As for the changes suggested initially here: I must admit that I've been thinking more about making changes along the lines of what you mentioned initially in this issue and that we discussed, but I'm still undecided as to the doing it. However, I have a structure in mind, should it happen. I'd basically allowed myself to procrastinate on making the decision with the expectation you were making the issue #134 changes and I shouldn't make sweeping changes that might affect those.

I pushed the changes I mentioned in my earlier comments into the "ME-Mak-next-version" branch, which was my main reason for asking you to base your changes on that branch and target your PR into that branch. I did it that way because I thought you were planning on beginning work shortly after that point, with the expectation of putting something together into a PR in the relatively near future, and I did not want to have multiple versions pushed out to everyone using it in rapid succession.

If you expect it to take a while for you to make changes, then I'll re-evaluate pushing those, by themselves, into master. I have no problem doing it either way, but if you expect to have changes in the near future, I'd prefer not to release multiple versions in rapid succession.

Hmmm.... as to the element names: It's certainly good to have those adjustments on Chem.SE. However, I have some concerns about applying them to SO. My concern is the possible use of element names as project, or product names. I have a vague memory of seeing a variety of element names used as project/product names, in which case they would be capitalized, rather than lowercase. However, I'm not coming up with examples off the top of my head.

However, that does not mean we shouldn't have the elements in there.

I had basically paused making major changes based on an assumption that you were going to make the changes you said you wanted to do in issue #134.

I am very sorry, I had lately been busy working on some other userscripts (SOX, and some of my own). I thought by the time I'd get over with them you'd push the updates you were planning to, but it seems you too thought the same. Sorry for the delay, and I'll get down to work immediately from today and hope to have #134 working most probably by tomorrow.

It's certainly good to have those adjustments on Chem.SE. However, I have some concerns about applying them to SO

I agree with you that it's not useful adding these adjustments onto SO. In fact, I was hoping to make them site specific only i.e. enabled only on a few sites (worldbuilding/physics/chemistry/etc.). In similar vein, the frameworks and languages (JavaScript, etc.) mentioned in the current Magic Editor are of no use to the science sites. At such a point, I feel like those custom science-SE site twirks might be better off in their own separate file, considering that the user base of Magic Editor is largely SO. However, at this point I realize I am far away from the original purpose of this issue. So, I'll start a new issue to work out these site-specific forks of Magic Editor,

Yep, looks like we miscommunicated. Don't worry about it. That happens from time to time.

Just to be clear, I don't want to push you into doing work. Everything here is voluntary. I'm happy to have you working on it and that you do it in your own time. If you're wanting to put it off, that's fine too.

SO-Close-Vote-Reviewers / UserScripts