Max URI length #20

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@fearphage
Contributor

fearphage commented Apr 8, 2011

Added a param to specify max URI length

-u, --maxuri length

This addresses half of issue #6.

@fearphage

This comment has been minimized.

Show comment Hide comment
@fearphage

fearphage Apr 8, 2011

Contributor

Related: Prevented the case where empty mhtml headers were added to the file.

Contributor

fearphage commented Apr 8, 2011

Related: Prevented the case where empty mhtml headers were added to the file.

@nzakas

This comment has been minimized.

Show comment Hide comment
@nzakas

nzakas Apr 18, 2011

Owner

I finally got around to looking at this. The changes look good but they're causing two unit tests to fail. That either means the changes broke some existing functionality or changed the functionality such that the tests are no longer valid. Can you take a look?

Owner

nzakas commented Apr 18, 2011

I finally got around to looking at this. The changes look good but they're causing two unit tests to fail. That either means the changes broke some existing functionality or changed the functionality such that the tests are no longer valid. Can you take a look?

@fearphage

This comment has been minimized.

Show comment Hide comment
@fearphage

fearphage Apr 18, 2011

Contributor

Can you confirm that they were passing before this change?

Contributor

fearphage commented Apr 18, 2011

Can you confirm that they were passing before this change?

@nzakas

This comment has been minimized.

Show comment Hide comment
@nzakas

nzakas Apr 18, 2011

Owner

Yes, and you can too my switching back to the current state of my repository.

Owner

nzakas commented Apr 18, 2011

Yes, and you can too my switching back to the current state of my repository.

tivac added a commit that referenced this pull request Apr 19, 2011

Issue #20
Fix testRegularUrlWithMhtml
Add testAbsoluteLocalFileUnderMaxSize
Add testAbsoluteLocalFileOverMaxSize
@tivac

This comment has been minimized.

Show comment Hide comment
@tivac

tivac Apr 19, 2011

Contributor

Fixed the tests, sorry about all the Git noise Nicholas.

Not quite sure if this was better or worse than the pull request in #25 but figured it was at least a bit more appropriate a venue since Phred did the majority of the work.

Contributor

tivac commented Apr 19, 2011

Fixed the tests, sorry about all the Git noise Nicholas.

Not quite sure if this was better or worse than the pull request in #25 but figured it was at least a bit more appropriate a venue since Phred did the majority of the work.

@nzakas

This comment has been minimized.

Show comment Hide comment
@nzakas

nzakas Apr 19, 2011

Owner

Cool thanks. I'll take a look when I get a free second.

Owner

nzakas commented Apr 19, 2011

Cool thanks. I'll take a look when I get a free second.

@nzakas

This comment has been minimized.

Show comment Hide comment
@nzakas

nzakas Apr 24, 2011

Owner

Merged in.

Owner

nzakas commented Apr 24, 2011

Merged in.

@nzakas nzakas closed this Apr 24, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment