{"action":"created","issue":{"url":"https://api.github.com/repos/shk3/edx-downloader/issues/162","labels_url":"https://api.github.com/repos/shk3/edx-downloader/issues/162/labels{/name}","comments_url":"https://api.github.com/repos/shk3/edx-downloader/issues/162/comments","events_url":"https://api.github.com/repos/shk3/edx-downloader/issues/162/events","html_url":"https://github.com/shk3/edx-downloader/issues/162","id":70590507,"number":162,"title":"We should refactor the code and have tests","user":{"login":"rbrito","id":212145,"avatar_url":"https://avatars.githubusercontent.com/u/212145?v=3","gravatar_id":"","url":"https://api.github.com/users/rbrito","html_url":"https://github.com/rbrito","followers_url":"https://api.github.com/users/rbrito/followers","following_url":"https://api.github.com/users/rbrito/following{/other_user}","gists_url":"https://api.github.com/users/rbrito/gists{/gist_id}","starred_url":"https://api.github.com/users/rbrito/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rbrito/subscriptions","organizations_url":"https://api.github.com/users/rbrito/orgs","repos_url":"https://api.github.com/users/rbrito/repos","events_url":"https://api.github.com/users/rbrito/events{/privacy}","received_events_url":"https://api.github.com/users/rbrito/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2015-04-24T05:23:49Z","updated_at":"2015-05-07T00:47:57Z","closed_at":null,"body":"Hi.\r\n\r\nI have been learning about tests lately. I already implemented some tests on my own fork of edx-dl, but I guess that it is better to have just one codebase (even though this project doesn't work the way that I like it to, I guess that we can reach some common ground).\r\n\r\nBack on the subject of tests, my other project, https://github.com/coursera-dl/coursera, has just switched from `nose` to `py.test` with the help of @meejah and @FedericoCeratto (read: \"they basically did all the work\"). I have learned a good amount of just by reading their code (and @meejah's code is very elegant, BTW) and this will surely stick with me.\r\n\r\nI still have *a lot* to learn about py.test (and I have been swamped with real-life events), but I guess that this project could use a lot of help by incorporating things learned from that transition.\r\n\r\n@shk3, @iemejia, any comments?\r\n\r\nRegards,\r\n\r\nRogério Brito.\r\n"},"comment":{"url":"https://api.github.com/repos/shk3/edx-downloader/issues/comments/99661048","html_url":"https://github.com/shk3/edx-downloader/issues/162#issuecomment-99661048","issue_url":"https://api.github.com/repos/shk3/edx-downloader/issues/162","id":99661048,"user":{"login":"rbrito","id":212145,"avatar_url":"https://avatars.githubusercontent.com/u/212145?v=3","gravatar_id":"","url":"https://api.github.com/users/rbrito","html_url":"https://github.com/rbrito","followers_url":"https://api.github.com/users/rbrito/followers","following_url":"https://api.github.com/users/rbrito/following{/other_user}","gists_url":"https://api.github.com/users/rbrito/gists{/gist_id}","starred_url":"https://api.github.com/users/rbrito/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rbrito/subscriptions","organizations_url":"https://api.github.com/users/rbrito/orgs","repos_url":"https://api.github.com/users/rbrito/repos","events_url":"https://api.github.com/users/rbrito/events{/privacy}","received_events_url":"https://api.github.com/users/rbrito/received_events","type":"User","site_admin":false},"created_at":"2015-05-07T00:47:57Z","updated_at":"2015-05-07T00:47:57Z","body":"Hi.\n\nOn May 05 2015, Ismael Mejia wrote:\n> First, sorry to answer so late. I agree, The current state of the project\n> is not the best, in particular the lack of tests makes the script too\n> fragile.\n\nGreat.\n\n> I really would like that we work in the automated tests, and that we can\n> learn a bit from your experience with the tests of coursera-dl.\n\nHave in mind that I don't know that much about automated testing.  I'm still\nlearning and I still have a lot to learn.  In fact, in a sense, I am using\nall these projects as testbeds for learning some practices that I never had\nany opportunities to experience. :)\n\nFor instance, one thing that I don't know is how to test things that need\nnetwork connections without making the people from edX or coursera getting\nmad.\n\nThis is needed for the recent changes in HTTPS (and Python versions changing\nit recently) which are giving me a headache in coursera-dl.\n\n> I really want this script to be better, but to be honest I really lost the\n> will to contribute bigger changes (like a decent refactoring) since the\n> last time I tried to do them. I know both of you had the best intentions\n> for the progress of the project when you told me to stop contributing my\n> changes and follow your proposed approach, but in that moment I had the\n> impression that your intentions were good, but that the execution was not\n> going to work, and today, almost a year later, nothing has improved and my\n> intuition was right.\n\nI agree. And I also value a lot your work. I guess that revisiting the\nsituation, as we are doing right now, is the way to go.\n\n> Plans and good intentions are good, but working code in the project is\n> better.\n\nDefinitely.\n\n> So I ask again if it is not better to do the changes in a progressive and\n> less 'formal' way (no branches, more work in the master, just break\n> it). This can be less stable for a while, but in the end it can be better\n> for the project, because that way we will have more eyes seen what is\n> happening, and a more open collaboration and testing (user-based but still\n> better than nothing).\n\nI guess that we can go with your changes. From my side, I have a few requests:\n\n* If you pick any commit from any other project of mine, use a `git\n  cherry-pick` or `git format-patch` and `git am` so that you preserve the\n  notion that git has of authorship (it treats the Author: and Committer:\n  fields separate).  This is important to get due credit and to have my work\n  recognized in statistics that employers may want to see (read: \"funding\").\n\n* Please, create *small* commits, for the sake of cherry-pick'ability and\n  bisecting. Small granular changes are key to use `git bisect` and healthy to\n  understand large changes.\n\n* Please, once you have a feature implemented, extract that thing and put it\n  into a separate function. This makes things easier to implement unit\n  testing.\n\n> Concretely about the tests, there is another issue. if we want to write\n> the unit tests, we need to have an API to test, so this is another\n> question, how are we going to agree about it, or  can you just begin to\n> move your changes into the master without breaking it. In that case I\n> agree to collaborate actively in any needed task.\n\nIdeally, I would love to just use as much of the same implementation as\npossible among different projects.  For instance, we may want to steal the\nfilename \"normalizations\" (for coping with filenames that have non-English\ncharacters and/or coping with file lengths).  I already implemented this in\na very bad manner in coursera-dl and it would be good to have edx-dl use\nthis.\n\nThe best implementation of this filename normalization that I saw comes from\nthe Picard project from musicbrainz.  I hope to grab that and use it for\ncoursera-dl.\n\nI may be exposing my ignorance here (since there must really be a better\nway), but I wish that Python dealt more easily with this.  In a similar\nsituation, our hacky overriding of print is sad and should be retired.\n\nRegarding testing, I would say, go ahead and just use Python's builtin\nunittest module.  Converting to other modules is not that difficult after\nall.  And, at least, we would have something to hook into a continuous\nintegration thing (e.g., Travis CI) so that future changes are tested\nautomatically at every push.\n\n> One last thing that you can disagree is that I really like the fact that\n> this script doesn't have any extra dependencies (apart of youtube-dl) and\n> I wouldn't like to add any other one that it is not worth (with probably\n> the sole exception of a test framework).\n\nNo, I'm not opposed to keeping the program as self-contained as possible.\nAnd if we ever split the program into modules, we can always resort to the\ntrick of creating an executable zip file.\n\n> I say this because I saw in your fork that you include some of them, but I\n> wouldn't like to add more complexity at least until we have a semi decent\n> stable version of the project with the tests.\n\nI don't quite remember. I haven't looked at the code of my fork for quite\nsome time. It has not been affected by various of the bugs that have been\nplaguing this project.\n\n> I really hope that we can collaborate this time, so just tell me what can\n> I/we do.\n\nAs far as I am concerned, go ahead and let's get this thing in a better\nshape. :)\n\n\nRegards,\n\n-- \nRogério Brito : rbrito@{ime.usp.br,gmail.com} : GPG key 4096R/BCFCAAAA\nhttp://cynic.cc/blog/ : github.com/rbrito : profiles.google.com/rbrito\nDebianQA: http://qa.debian.org/developer.php?login=rbrito%40ime.usp.br\n"}}