Refactor #165
Refactor #165
Conversation
Codecov Report
@@ Coverage Diff @@
## master #165 +/- ##
=========================================
+ Coverage 73.43% 75.64% +2.2%
=========================================
Files 10 23 +13
Lines 256 312 +56
Branches 0 29 +29
=========================================
+ Hits 188 236 +48
+ Misses 68 65 -3
- Partials 0 11 +11
Continue to review full report at Codecov.
|
robwise
force-pushed
the
robwise/refactoring
branch
from
427da3c
to
1832af6
Compare
|
@darahak I have Q/A'd this locally, do you think you could check it out too? The changes are so deep that I'm just worried about it because it's like launching from scratch. I'm having some of my coworkers beta test it as well. |
robwise
force-pushed
the
robwise/refactoring
branch
from
1832af6
to
f159e9f
Compare
|
@robwise Sure, I'll take a look! |
decls/index.js
Outdated
| @@ -36,6 +36,8 @@ declare type Atom$Disposable = any; | |||
| declare type Atom$View = any; | |||
| declare type Atom$Workspace = any; | |||
| declare type Atom$Command = { name: string, displayName: string }; | |||
| declare type Atom$Notifications$Options = { detail?: ?string | ?Error, dismissable?: ?boolean }; | |||
There was a problem hiding this comment.
I'm wondering if we should stick with the official documentation for these type declarations?
That would mean detail is string only, and we have to explicitly "cast" errors to strings.
| @@ -0,0 +1,3 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`getPrettierOptions() returns all prettier options 1`] = `undefined`; | |||
There was a problem hiding this comment.
Is it supposed to still be here?
It tests against undefined and I don't see it being used in index.test.js.
There was a problem hiding this comment.
roger, I deleted these snapshots and re-wrote from scratch
src/displayDebugInfo/index.js
Outdated
|
|
||
| const getDepPath = (dep: string) => path.join(__dirname, '..', '..', 'node_modules', dep); | ||
|
|
||
| const getDebugInfo = () => ` |
There was a problem hiding this comment.
Probably nitpicking: this adds newlines at the top and bottom of the message.
But no need to change if the notification looks okay.
There was a problem hiding this comment.
done. fixed using .trim()
| // $$FlowFixMe | ||
| prettierPackagePath ? require(prettierPackagePath) : bundledPrettier; // eslint-disable-line | ||
|
|
||
| const getPrettierInstance: (editor: TextEditor) => typeof bundledPrettier = _.flow( |
There was a problem hiding this comment.
This line is confusing to me because of (...) => typeof bundledPrettier = ....
Is it specific to Flow?
There was a problem hiding this comment.
Yes specific to flow, it's an inline typing of the getPrettierInstance const. Because calling _.flow() returns a function, I don't need to wrap it with another arrow function. However, lodash/fp doesn't have flow-typed support, so I am explicitly defining the API. This says getPrettierInstance is a function that takes a TextEditor as the only argument and returns something that has the same type as the bundledPrettier object.
robwise
force-pushed
the
robwise/refactoring
branch
from
f159e9f
to
1399ce7
Compare
|
@darahak thanks bro! I fixed the stuff you pointed out. Beta testing went well, just one issue due to a typo so far (which I fixed). If you're curious about learning FP, I find it much easier to read if you do it right once you get used to it. Check out this free guide: https://drboolean.gitbooks.io/mostly-adequate-guide/ He uses ramda but |
As the codebase has scaled, the src folder has become increasingly large and unwieldly. Additionally, helpers and options had become quite extensive and lacked cohesion. This commit refactors the various concerns into a nested folder structure. Functions that are only needed to assist a specific domain are contained within that domain's folder. Helpers that interact with atom are now in an atomInterface folder so that there is a clear interface to atom and so that it is easy to mock accessing atom during testing. The same goes for helpers that interface with the Atom$Editor object. Helpers are just those low-level functions that need to be shared between different domains. We also move away from the strict "actual"/"expected" syntax for tests, although this may still be used if expectation objects are considerably complex.
robwise
force-pushed
the
robwise/refactoring
branch
from
1399ce7
to
e1a9b39
Compare
|
@robwise Thanks for the suggestion! I actually used Ramda for one project, so it should be fine. |
This is a substantial refactor of the codebase. We had started to amass too many files in the root of the src directory, and the helpers file had become very incohesive. Now, if a certain function is only used once and does not interface with atom or the atom TextEditor object, it stays near the function calling it either in the same file or as a sibling file. Each high-level operation or concern is given its own folder so that the root of src is much cleaner and easier to read.
Other changes of note:
actual/expectedsyntax for tests as this was unusual for people trying to contribute to the project, and I'm not 100% sold on it personally anyway$FlowFixMeannotations everytime we usedmockImplementation. That noise was probably hurting more than flow was helping.