[core] Consolidate app detail logic into single module #705
Conversation
Remove both `getAppName` and `getReleaseChannel` into `getAppDetails`. Move consumers over to new module. Rewrite config file path logic into shorter function, and move known users of this logic to new module
… `atom-application.js`
There was a problem hiding this comment.
Looks good, I verified that (to my eyes) the transposition of the functions out of those other files into get-app-details.js was correct, logic appears effectively the same, to my eyes.
Just had a question about the new stuff from this PR, and it's looking good generally.
I'm into the idea of consolidating these related modules into one file. 👍
There was a problem hiding this comment.
Approving with or without refactoring the two functions together. (As long as no bugs are introduced during any refactors 😉).
Seems like a good consolidation of related stuff, and helps us define stuff once and use multiple places, which as I understand it is a widely accepted coding best-practice™️.
👍
|
Thanks a ton for the review @DeeDeeG! I did go ahead and refactor to combine the two functions, I'll wait to see if tests are happy before merging with your approval. Appreciate it! |
|
Looks like tests are happy here, and with approval lets get this one merged |
Currently there are many instances of duplicated logic within the codebase, especially when it comes to finding the configuration files of Pulsar, or iconography of Pulsar. So especially if we want to eventually support the XDG specification, we need to consolidate this logic into a single location. Additionally, having important pieces of logic like this centrally defined allows us greater control over any changes that may need to be made here in the future, as well as having a better understanding of exactly how this works, in any given instance.
There are already two important pieces of similar logic that have their own, very small, modules, that is
get-app-name.jsandget-release-channel.js. So what I've done is moved both of these modules into a single module namedget-app-details.js.Moving these into the same module seemed to make sense due to how small they are, and that they both fit conceptually as
app-details.The most important aspect of this PR is that I've then searched through the codebase for any logic of attempting to find the Pulsar configuration file. I was only able to find two instances, with somewhat similar logic, albeit they were different sets of logic.
So I've rewritten the logic used to find the configuration file, taking after the same code pattern used in
packages/autocomplete-html/update/update.jswhich allows a super cool, simple way to search through several possible file declarations and find the one that exists. Using that I've made the new functiongetConfigFilePath(), that is also exported byget-app-details.js.Which of course, I've then moved the logic of finding the config file out of these separate modules into this new one. So now if we ever needed to make any changes here, or search new locations, this is centrally defined.
I really do hope we can further use this new module to centrally define any other pieces of important data, such as iconography, or more importantly find anywhere else we attempt to locate config files, the app name, and so on.
Relates to #338, #390