[core] Combine constant values

[confused-Techie]

Much like #705, this PR aims to reduce the amount of duplicated data throughout the Pulsar codebase.

Although instead of focusing on values derived from functions, this PR focuses and raw values throughout the codebase. And moves all of them to pulsar-constants.js.

This module exports several values like the PROTOCOL ("atom:"), and different URI paths all with the goal of making sure we can declare these values once, and have them updated across the entire codebase.

[DeeDeeG]

Good idea overall! I appreciate the notion of it. Makes sense.

Some minor thoughts about the constant names. Wouldn't mind your thoughts on this feedback, but not to hold this PR up too long.

If a strong argument can't be made for changing the names (and I'm not certain my suggestions were strong ones to change the names), then I will lean toward approving this as originally written/without changes and giving it the thumbs-up.

I'm actually having my doubts as I'm typing this whether changes are worth doing. But wanted to give it a minute and a chance for discussion. Otherwise leaning toward an approve.

[EDIT: Also I see some failing tests, obviously that should be resolved before merging, just noticed that! Maybe I can help resolve those one of these days, though it's not top of my personal to-do list for Pulsar at the moment.]

[DeeDeeG]

Maybe this?

Suggested change

	PROTOCOL_NAME: "atom",
	PROTOCOL: `${this.PROTOCOL_NAME}:`,
	PROTOCOL_PATH: `${this.PROTOCOL}//`,
	PROTOCOL: "atom",
	PROTOCOL_COLON: `${this.PROTOCOL}:`,
	PROTOCOL_PATH: `${this.PROTOCOL}://`

Since the colon doesn't really have a name or belong to any specific segment of the URL, it's a delimiter or separater but it's not part of the protocol/scheme, it's technically just a separator between the protocol/scheme and the next bits of the URL.

Or have consumers of this file write out the ${CONSTANTS.PROTOCOL}: themselves and not define a PROTOCOL_COLON constant here?

Also, not to be too pedantic, but I think this atom:// is technically more correct to call PROTOCOL_AUTHORITY than PROTOCOL_PATH? https://en.wikipedia.org/wiki/URL#Syntax

Is it too silly to call it PROTOCOL_COLON_SLASH_SLASH?? Or have consumers of this file write out the :// themselves and not define this constant either? Anyway, if having it be a constant here is useful I will try to avoid bikeshedding the name too hard.

(Maybe to be technically correct PROTOCOL should be SCHEME... http is a real protocol, but atom is really just a URI scheme... but I honestly don't like it as much and I think it's intuitively/colloquially clear as-is, so I don't love the thought of calling it SCHEME actually...)

[confused-Techie]

I personally would be totally fine with calling this PROTOCOL_AUTHORITY and the same goes for technically correct naming on everything else, such as changing it to PROTOCOL_SCHEME. I know they are a bit longer, and may not be as pleasant to look at, but it is the correct naming for things. I just hadn't considered to lookup the information on the nomenclature, so went with my gut on these things.

[DeeDeeG]

Oof, now I see why atom: was named CONSTANTS.PROTOCOL.

Maybe the right move is to call it CONSTANTS.PROTOCOL after all, just to make working with NodeJS's built-in url module nicer?

[confused-Techie]

Yeah, this is exactly why I declared each portion of it differently, so that different places could use it as needed.

[confused-Techie]

@DeeDeeG Thanks for taking a look at this. I did break out each part independently since different parts are used in different locations. But like you mentioned a couple times of making each user of the constant declare things like : or // or :// is honestly fine by me.

It depends on our priority for use, should it be easier to get the constant you want? Or easier to remember the constants. Since unfortunately I can see use needing to recheck the constant list to know if we should use PROTOCOL_PATH or PROTOCOL etc, meanwhile just knowing that PROTOCOL or SCHEME exists as the text of atom (Or whatever it may change too) becomes very easy to remember how to get the value you want, while giving the user the responsibility to add any other text needed. So the argument could easily go either way, and I'm not attached to the current implementation, so if anyone thinks it's best to keep things simple I'll happily change it.

But you are right, tests are failing, and I honestly don't know why. They all relate to handling of the URI, which means something about the way we are providing the URI isn't working, and I'm not sure where it's failing, beyond the templated strings not liking being nested, or not liking being in strict mode or something.

[confused-Techie]

In the end I opted to leave more constants rather than fewer. While this may mean it's harder to know them off the top of your head, I took DeeDeeG's naming advice which may hopefully help to make things very obvious on what's what.

Using names like PROTOCOL for just atom then PROTOCOL_COLON for atom: and PROTOCOL_COLON_SLASHES for atom://. With the name being extremely obvious. Unfortunately this section of the URI doesn't have an official name as far as I can tell, but at least nobody will be confused by what's in use here.

[DeeDeeG]

One last thing, if we're doing this for forkability... Something crossed my mind...

We opted against changing the scheme away from atom:// to pulsar:// to avoid fragmenting the ecosystem and causing link-rot, basically. (Pulsar is pretty much acting as a drop-in replacement and doesn't need to change the URL scheme, basically.) Should we consider not making the protocol things consts in general, so as to not encourage forks to absent-mindedly change them without realizing they're un-subscribing from any extant links and fragmenting the links/integrations ecosystem?

As I said, something that just crossed my mind. Curious what others think.

(On the other hand, "one source of truth" is usually good in programming, and even without changing away from atom:// I can see the case for it discouraging typos. Since CONSTANTS.PRUTOCOL is not defined is immediately apparent, rather than trying to notice one wrong character in an otherwise valid JS string.)

(On the other other hand, knowing the difference between CONSTANTS.PROTOCOL and CONSTANTS.PROTOCOL_COLON is probably harder by degrees than visually scanning atom vs atom:... Hmm, now that I'm typing it out hopefully the difference is pretty clear there? So at least the naming choices seem pretty readable IMO.)

[savetheclocktower]

(On the other other hand, knowing the difference between CONSTANTS.PROTOCOL and CONSTANTS.PROTOCOL_COLON is probably harder by degrees than visually scanning atom vs atom:... Hmm, now that I'm typing it out hopefully the difference is pretty clear there? So at least the naming choices seem pretty readable IMO.)

I've always been very slightly 👎 on this change for that reason alone, though ultimately I didn't care enough to go twelve rounds over it.

My feeling is that it does make the code a bit less readable for not much benefit. I don't see it as preventing bugs — there's a discoverability issue here and nothing to prevent someone from typing "atom://" in a future pull request. And if a would-be bug was caught by enforcement of these constants, that suggests to me that someone wasn't writing specs for their code, or else that bug would've been caught a different way.

I only just now realized how few files are touched by this PR — suggesting to me that we've already got a healthy separation of concerns, and that this isn't a problem that we really need to solve.

I'd be neutral-at-worst on a version of this PR that tried to be less comprehensive — maybe only PROTOCOL, DOT_FOLDER, and the config/snippets/stylesheet/init files — without all the other stuff. But ultimately I'd still wonder if there was a point.

[confused-Techie]

That's totally valid if we don't think this is needed. I personally started looking at this due to concerns of having data defined in multiple locations, that if it was ever updated, I'd worry somewhere was missed.

Really the crux of wanting to make a change like this was the hope that it would help us abide by the XDG spec, but I know this PR does little to nothing of that particular goal.

But if we feel like this is added complexity and having to sift through one extra file for not a lot of benefit, I'm fine with use closing this one out instead.

[DeeDeeG]

Would also be slightly in favor of closing as well, if that's alright.

On basis of complexity of the change vs hard-coding, with hard-coding seeming to me to be a (subjectively, somewhat) better trade-off in this case, with few uses of the constants in relatively few files. (My take at this point, basically concurring with @savetheclocktower).

[confused-Techie]

Fair enough then, if it doesn't actually reduce complexity for the majority of people then no reason going any further with it.

Thanks for the input @DeeDeeG & @savetheclocktower