These are chat archives for daviferreira/medium-editor

7th
Apr 2015
Peter E Higgins
@phiggins42
Apr 07 2015 00:16
@nmielnik ping?
Peter E Higgins
@phiggins42
Apr 07 2015 02:36
12 left.
Nate Mielnik
@nmielnik
Apr 07 2015 02:39
So I don't think moving into amd modules (or specifically requirejs) should be part of this set of changes. That's a whole other separate discussion
I think the same goes with removing the src + dist files, it's a separate discussion and effort
Peter E Higgins
@phiggins42
Apr 07 2015 02:45
yah that's neither here nor there, i just needed a way to easily dynamic load a bunch of files and not fuss with tricking lint. also it isn't actually a conversion to amd or requirejs, there is a shim in the wrapper that makes it entirely internal and is very little overhead, no different than the "fake" concat'ing and jslint'ing globals that was in place
also src and dist removal was pragmatism to prevent conflicts when switching branches, in no way intended to suggest they should be removed outright
the amd->fake-global conversion is mindlessly easy to back back out.
as easy as it was to put in
Nate Mielnik
@nmielnik
Apr 07 2015 02:46
Aligning our other internal objects, like pastehandler, could be handled in individual PRs for each component too
Peter E Higgins
@phiggins42
Apr 07 2015 02:46
var Thing; (function(){ Thing = {} })
versus define([], function(){ return THing; }) // nobrainer to convert
Nate Mielnik
@nmielnik
Apr 07 2015 02:47
Same with each set of options that we convert over
Peter E Higgins
@phiggins42
Apr 07 2015 02:48
sure, each of them could be done incrementally but I have already done them all in bulk so what is the issue?
Nate Mielnik
@nmielnik
Apr 07 2015 02:50
it's not possible to look at each of the separate things in isolation, and talk about the best way to do the individual parts
Peter E Higgins
@phiggins42
Apr 07 2015 02:50
it is literally 98% backwards compatible in the current form. no one would notice. even the AMD argument you just made, that is all internal and not exposed. end result is a medium-editor.js file that you can <script src="..."></script> just like you can now, but with the added benefit of people who do use amd can point a namespace direct at the src/js/ tree, and pick and choose and hack and work.
no, it is not possible to look at them in isolation.
i will be sure to squash commit the whole thing, and add commits against whatever version is so that can be true.
i need to do that anyway, history would be tracked better if I'd done proper git mv instead of just blindly replacing files
so much is untouched its ridiculous :)
95% of the changes are this.options.foo -> this.foo and the likes
Nate Mielnik
@nmielnik
Apr 07 2015 03:01
so it's not about restructuring the commits, it's about being able to work through small parts as a group, and learn from those changes and discussions, and apply that to the next set of changes going forward
but more importantly, have it be a group discussion that we can handle in small doses, as opposed to a large refactor from 1 person that is hard for us to collaborate about
For example, if you had you proposal for how we change options implemented for a single group of options, we could get that in, and then I could model my PR on top of that
and then maybe we can do all the other options in a 3rd PR after that, once we've basically flushed everything out
Peter E Higgins
@phiggins42
Apr 07 2015 03:05
from my POV i suggested a sweeping change, drew up a lenghty plan of attack, got nothing but +1's and mild discussion, incorportated those limited discussions into a POC branch that, over easter weekend, actually went out and implemented, and managed to get 95% of the regression tests passing before even mentioning that I had a POC going.
you can break it into as many parts as you like. i was simply doing discovery and it turned out to match the proposal and i snuck in backwards-compatibility as a bonus feature.
my work was based on the "step back and thing about 5.0" trigger I kept mentioning
backwards compat was just a bonus that happened to be as a result of the way I did said discovery.
Nate Mielnik
@nmielnik
Apr 07 2015 03:13
so I am definitely not suggesting that your set of changes is invalid or doesn't align with the things we had talked about
I'm just saying that it's very difficult for someone like me to digest all of the things you have in place and try extract individual parts to discuss
Nate Mielnik
@nmielnik
Apr 07 2015 03:19
As opposed to having a pull request like this sit around for a long period of time, and need to be tinkered with and kept up to date by yourself individually (with limited visibility for the rest of us)...I submit (maybe selfishly) making small PRs that can be read over and agreed upon in a small chunks of time will make eventually getting to that end state you have not just make someone like myself understand and feel better, but gives us all a chance to have input along the way
Peter E Higgins
@phiggins42
Apr 07 2015 03:24
maybe I just spoke too soon. i want to sit and talk about the proposal as a whole, with any level of discussion in the interim. i am so close to presenting a full back-compat alternative end-result file (irregardless of any supporting lib or code needed to make it happen in the POC) that I may have been overzealous and excited to present it that I saw a conflicting implementation of "fancy extension args" as a threat (or more work that i'd have to do to get 100% compat again).
i would love to break the entire changeset down into individual line-item discussions
there is very little "sweeping" change about it actually, other than files-moved-around ... the diff breaking buttons.js into default-defintions and base-class is going to be ugly no matter how it happens
and i broke it into parts because it is more manageable that way to me, as I was the only one working on my random idea over random weekend. i was surprised and impressed with how easy it was to make a project use amd internally and not actually expose it.
90% of changes, as I mentioned, are really in this.base.options.foo -> this.foo and discovering where those parts overlap.
Peter E Higgins
@phiggins42
Apr 07 2015 03:30
plus other random features that will likely back out before a real PR is even made? did you notice the change from firstHeader: a, secondHeader: b to a default of headers:[a,b]
giving people the ability to define up to 6 while maintaining the default of 2 @ h3/h4?
Nate Mielnik
@nmielnik
Apr 07 2015 03:33
ok, ultimately I don't want to just be a road block, and I think every line of code you write is valuable and want to find ways to get that kind of effort into the codebase as easily as possible. I don't know that we can do that with a POC that requires an all-or-nothing PR, but if you think making a POC and then opening iterative PRs separately works well for you then I'm for it.
No, I didn't notice that, and that's part of the reason why I would love for that kind of thing to be a one-off PR that can be evaluated and merged on its own and as fast as possible, without having to talk about things like amd, requirejs, extensions, exposing utility methods, prototype inheritence, etc.
anyway, these are my thoughts with the limited amount of time I've had the past few days. It's entirely possible that Davi and/or Noah don't agree with my suggestion and will be fine with merging your POC into our codebase quickly.
in which case, I've just wasted your time and I don't want that at all
either way, I'm excited with the direction, and really appreciate the energy you've injected into our project, as well as the tons of code you given me to read through
Nate Mielnik
@nmielnik
Apr 07 2015 03:38
thanks, and I'll try to chime in tomorrow or the next day
Peter E Higgins
@phiggins42
Apr 07 2015 03:38
chicken and the egg. A lot of what I've done is pragmatism suiting my personal needs while accomodating the community-at-large/regression-tests. and yes, my time is superimportant to me, for instance I am going to now go feed by 1mo-old son and pray for a whole night of sleep.
Davi Ferreira
@daviferreira
Apr 07 2015 06:49
tbh I was expecting a much smaller scope, but I like it
I just need to review it with more time
and I just agree with Nate, I don’t feel very comfortable with such a massive change :D
even with all the test coverage, but its not about that, its more about us understanding what was changed and how to do things
Peter E Higgins
@phiggins42
Apr 07 2015 12:02
again it only seems like a massive change because files moved around. the core of it is straight c/p of previously exposed/hidden functions. the only change in real life is how the instantiation options are parsed, and that's all confined to a single file addition w/ one function called "bymagic"
if I c/p them back over top of the original localtions of the files and then use git mv to move the files they will appear as proper diffs, which would expose the the majority of the changes are this.base.options.configName -> this.configName