Skip to content

Fixes #10#11

Closed
erfaan wants to merge 1 commit intopeterbe:masterfrom
erfaan:master
Closed

Fixes #10#11
erfaan wants to merge 1 commit intopeterbe:masterfrom
erfaan:master

Conversation

@erfaan
Copy link

@erfaan erfaan commented Feb 8, 2013

There is no need to manually join the urls when we have urljoin

@peterbe
Copy link
Owner

peterbe commented Feb 11, 2013

Do all tests work with this?

I'm wondering if I didn't use urlparse.urljoin for a reason. I can't remember.

@peterbe
Copy link
Owner

peterbe commented Feb 12, 2013

I can't merge this without tests but I'll take a look if you don't have time to write some.

@peterbe
Copy link
Owner

peterbe commented Feb 12, 2013

I did find a flaw in the proxy/app.py script to do with script tags. Look at it now.
http://cl.ly/Mqcb

@erfaan
Copy link
Author

erfaan commented Feb 13, 2013

Yes all tests ran smoothly. Just tested it. The issue is not related to proxy/app.py but the processor.py
What exactly did you change in proxy/app.py?

I am not sure how you managed to make it working. I tried the following code and it failed:

>>> from mincss.processor import Processor
>>> p = Processor()
>>> p.process_url('http://www.i.com.pk')
>>> p.process()

Regarding urljoin, I guess that is the proper way to join urls.

@peterbe
Copy link
Owner

peterbe commented Feb 13, 2013

Ok. Great I can reproduce that now.

I still can't merge your pull request if it doesn't have tests.

And lastly, please don't teach me about how urljoin works.

@peterbe
Copy link
Owner

peterbe commented Feb 13, 2013

You were right. It works with plain urljoin. I can no longer find any reason why it wouldn't work. And I added some unit tests too.
2786c69

@peterbe peterbe closed this Feb 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants