Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add width attribute #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Utkarsh1308
Copy link
Contributor

@Utkarsh1308 Utkarsh1308 commented Jun 9, 2019

Adds width attribute which allows user to
specify number of bytes printed per line

Closes #3

@Utkarsh1308 Utkarsh1308 changed the title Adds width attribute Add width attribute Jun 9, 2019
Copy link
Owner

@juhakivekas juhakivekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss this in the issues before merging :)

test/width_test.py Outdated Show resolved Hide resolved
test/cli_test.py Show resolved Hide resolved
multidiff/command_line_interface.py Show resolved Hide resolved
@Utkarsh1308 Utkarsh1308 force-pushed the width branch 6 times, most recently from dc4ec66 to 1d82f23 Compare June 13, 2019 13:30
@Utkarsh1308
Copy link
Contributor Author

Please Review :)

@Utkarsh1308 Utkarsh1308 force-pushed the width branch 5 times, most recently from e2ff084 to 8a64212 Compare June 13, 2019 13:56
test/cli_test.py Outdated Show resolved Hide resolved

def final(self):
self.hexrow += 3*(16 - self.rowlen) * ' '
self.asciirow += (16 - self.rowlen) * ' '
self._newrow()
return self.body

def reformat(self, body, n=16):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to be this complicated. Just find the number 16 from Render.py and make it not hardcoded. For example replace all occurencies of 16 with self.bytewidth and set that variable when constructing the Render object. This way there's no need to compile the regular expressions, find strings and do all the reformatting that takes a lot of time (you mentioned this was an issue?). As a side effect the format (addresses, ascii conversion etc.) will match the original

Copy link
Contributor Author

@Utkarsh1308 Utkarsh1308 Jun 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I tried the approach you are suggesting first but then the code runs slowly on my machine. I don't know if it's a problem with my machine or not. Please tell me if by changing those lines the code runs the same in your machine. I would be happy to implement that since it's easier :)

@Utkarsh1308
Copy link
Contributor Author

Please review

@Utkarsh1308 Utkarsh1308 force-pushed the width branch 4 times, most recently from a16cb80 to 2b62b0f Compare June 14, 2019 11:48
Adds width attribute which allows user to
specify number of bytes printed per line
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.

Consider making output as wide as the terminal
2 participants