Code review comment for lp:~diogenesethecynical/lernid/Bug793793

Revision history for this message
Diogenese TheCynical (diogenesethecynical) wrote :

Hi John

Sorry for my late reply. I understood what you are trying to say. I have made the appropriate changes. Before making these changes I thought I will update to the latest code and after that, I cannot launch Lernid . I got error (ImportError: No module named gtkmozembed)

I am looking into how to solve it (quick search revealed install python-gnome2-extras, but that did not solve this , I am looking into it however. Probem should be of my environment if others can work with the same code.

But I thought I will fix the code and get it reviewed by you before asking for merge. Here is what I plan to do:

from urlparse import urlparse

def set_location(self, url):
        o = urlparse(url)
        if o.scheme == '':
            url += 'http://'

Please let me know your thoughts on this. Once you give a go, I will formally ask you to merge.

Thanks for all your help .

Regards.

________________________________
From: John S. Gruber <email address hidden>
To: <email address hidden>
Sent: Monday, July 25, 2011 10:57 AM
Subject: Re: [Merge] lp:~diogenesethecynical/lernid/Bug793793 into lp:lernid

Thanks for the comment, Diiogenese. That's not what I had in mind, and
I think it would be a bad idea, frankly.

I had in mind the possibility of using the python urlparse function
(as in import urlparse), and have it parse the
user's input, to see if it parses a scheme, the scheme is the http: or
ftp: part of the url. If it doesn't then you
could append http:// to what the student asked for.

When programming in python I usually have a browser tab open to the
python library documentation, and when
programming in lernid I usually have another tab open to look at the
pygtk documentation. You should be able
to google for these. Another useful thing is the following: if looking
for a particular item, such as the string "label"
you can use fgrep -ir label *  when sitting in the lernid top
directory to search the whole project for the string "label" (or
"LaBel", the -i is for case-insensitive and the -r option is the
recursive option. I also often will open a separate terminal
window and run python in it. Then I might say "import urlparse" and
then help(urlparse) or dir(urlparse) to see what's in the module. I
might then run it through some examples in that window until I think I
have good code to put into lernid. It can
save time. You might already do these things, of course, or have
better ways of working.

Some urls don't contain www.  -- that's just a computer naming
convention. I often use just http://launchpad.net or
http://gmail.com, myself. The http: or https: is what makes something
a web url, and the SOMETHING: at the head
of the URL is the scheme, the protocol to be used to retrieve the
information the URL specifies.

Thanks.

John

On Sun, Jul 24, 2011 at 10:14 PM, Diogenese TheCynical
<email address hidden> wrote:
>> This isn't for sharing a url, however, it is a shortcut for someone using the
>> Open Url... I don't think the browser will work with ftp:// ,etc., at this
>> point anyway, but I guess it could in the future.
>>
>> Diogenese, you could check have urlparse do some work here and have it find
>> the scheme. If it finds no scheme you could do the prepend of http:// . That
>> might make the code more robust. I'm not sure this is terribly important.
>
> Hi John
>
> You are absolutely right. We might need to support ftp:// in future.
> What do you think about this:
>
> if URL starts with www then preappend http:// to it.
>
> If this is not what you had in mind when you said find the scheme, please do let me know.
>
> Regards.
> --
> https://code.launchpad.net/~diogenesethecynical/lernid/Bug793793/+merge/66785
> You are requested to review the proposed merge of lp:~diogenesethecynical/lernid/Bug793793 into lp:lernid.
>

--
https://code.launchpad.net/~diogenesethecynical/lernid/Bug793793/+merge/66785
You are the owner of lp:~diogenesethecynical/lernid/Bug793793.

« Back to merge proposal