-
Notifications
You must be signed in to change notification settings - Fork 0
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
remove a bunch of ramda #964
Conversation
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 good! I'm looking forward to getting rid of ramda.
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.
So far, this is great work @hejkal, thank you so much!
I've got some more to look through but I think this PR is actually too big already. The lack of front end and integration tests makes this really hard to guarantee we're not breaking anything. It's obviously good practice in general but given that it's open source, we have a secondary reason: if I (or theoretically anyone) wants to upstream any of the work, it becomes harder to manage cherry-picking. As I often say, doing things in the open incentivizes good engineering practices 😄
All that said, outside of a few things I've mentioned, I don't think it make sense to do go back and separate this all. Let's wrap up the work that's already been done and do additional changes in separate, more easily digestable and potentially reusable PRs.
const emailLists$ = flyd.map(R.prop('body'), request({method: 'get', path: pathPrefix + '/email_lists'}).load) | ||
state.selectedTagMasterIds$ = flyd.map(R.map(ls => ls.tag_master_id), emailLists$) | ||
const emailLists$ = flyd.map(r => r.body, request({method: 'get', path: pathPrefix + '/email_lists'}).load) | ||
state.selectedTagMasterIds$ = flyd.map(emailLists$.map(ls => ls.tag_master_id)) |
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.
This one is wrong, I think.
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.
Can you say more about what looks wrong here? As far as I can tell it's the same as other similar places, switching from R.prop('thing')
to i => i.thing
and from R.map(f, thing)
to thing.map(f)
4421251
to
3316d90
Compare
02415bf
to
b6f19fd
Compare
closing this because I split it out into smaller chunks |
NOTE: DO NOT discuss internal CommitChange information in your PR; this PR will be public.
Link back to the issue in the Tix repo when you need to do that.