New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refact records #2588
Refact records #2588
Conversation
This is a precursor to moving the file. It simplifies the move to only need to track a couple things that are actually used.
This is a cheat-y commit, I missed fixing some tests that were likely broken from previous commits. I.e., I'm skipping the messy step of rebasing all those edits back into the previous commits.
@rgbkrk A little overdue, but here you go 😅. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking super great here!
@@ -4,7 +4,7 @@ import { remote } from "electron"; | |||
import { from } from "rxjs/observable/from"; | |||
|
|||
import * as nativeWindow from "../../src/notebook/native-window"; | |||
import { makeAppRecord, makeDocumentRecord } from "@nteract/core/records"; | |||
import { state as stateModule } from "@nteract/core"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be really tempting to call the state module "core".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, yah that's true. We should figure out the right names for all this stuff soon. I like that all this is under /state
though, it makes it really easy to manage in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah my only worry is the super common use of the variable named state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, definitely worthwhile to not block that variable name 😆.
Thanks for the quick review! |
Howdy! I'm 🔓🤖! In order to keep information timely (based on the most recent release), we want all activity to be added to either new issues or open issues and PRs. In service to that goal, I, the lock bot close inactive closed issues when they haven't had activity in 120 days. Feel free to open a new issue for related bugs and link to relevant comments from this thread. |
Woo! After some of the recent refacts, this change became much easier. I nested
records
atstate/old
.I think we still need to think about naming and what we want to expose as a top-level export. However, we're getting a lot closer to the organization we're ultimately looking for in
/core
.