-
Notifications
You must be signed in to change notification settings - Fork 95
add html formating #92
base: master
Are you sure you want to change the base?
Conversation
|
Looks appealing to me, but any browser will struggle with very large exports (I tried with a 145MB file and stopped Firefox at 4.4GB of memory used). Maybe we would need some sort of pagination? I also found that |
Co-Authored-By: mr-tron <[email protected]>
|
Yea, but we can start from small improvments, i think. |
|
I really appreciate you taking the time to work on this @mr-tron, thank you. This has been a feature we've been discussing for a long time. |
|
Do you plan to merge this pull request? |
|
Yes, let's start with small improvements as you said. We can build on this. |
|
Some improvements that need to happen before this can be merged:
I'll use GitHub's tools to annotate specific lines. Once again, thank you so much for working on this. The reason I ask for these changes is not because I think you've done a bad job, but because I want this code to be a foundation for great things and that starts with fixing these nags. |
| row = cur.fetchone() | ||
| if not row: | ||
| return None | ||
| row = row[:1] + (datetime.datetime.fromtimestamp(row[1]),) + row[2:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See how the other functions do this: they construct the namedtuple first, then use something like _replace(original_date=datetime.datetime.fromtimestamp(original_date)
|
|
||
| def get_human_readable_forwarded(self, forward): | ||
| if not forward: | ||
| return "Unknow forward" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here.
| if forward.from_id: | ||
| ent = self.get_user(forward.from_id) | ||
| if not ent: | ||
| return "Unknown user: %s" % forward.from_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change these % formats to use "{}".format() style formatting
| row = row[:1] + (datetime.datetime.fromtimestamp(row[1]),) + row[2:] | ||
| return Forward(*row) | ||
|
|
||
| def get_human_readable_forwarded(self, forward): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in my comment, I think this should be moved to a higher level (i.e. htmlformatter)
| from . import BaseFormatter | ||
|
|
||
| from .. import utils as export_utils | ||
| from .htmltemplates import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import * should be avoided if possible. I understand that you want to import a lot of names for templates, but it's totally fine to have a long import line like
from .htmltemplates import column_width, styles, header, basic_message_template, normal_body_template
Alternatively, use a namespaced import so you can do things like templates.styles, templates.basic_message, templates.normal_body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@expectocode you should try the new beta feature from GitHub
```suggestion from .htmltemplates import column_width, styles, header, basic_message_template, normal_body_template ```
| media_path = 'usermedia/%s-%s/%s' %(self.get_display_name(context), context.id, media_path) | ||
| body = photo_body_template.format(img_path=media_path) | ||
| else: | ||
| text = "Unsupported media type: %s. Message_id: %s, media_id: %s" % \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
% formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With inconsistent spacing. But yes, use .format().
| </div> | ||
| """ | ||
|
|
||
| forwaded_wrapper_template = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| forwaded_wrapper_template = """ | |
| forwarded_wrapper_template = """ |
| """Format the given context as HTML and output to 'file'""" | ||
| entity = self.get_entity(context_id) | ||
|
|
||
| # file = sys.stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should have been deleted
| border-bottom: 1px solid #e3e6e8; | ||
| } | ||
| """ % (column_width, column_width - 20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
% formatting
| @@ -0,0 +1,150 @@ | |||
| column_width = 480 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a comment about what this controls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not totally necessary, just a thought. Of course, comments aren't the best solution as they can become outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments aren't the best solution as they can become outdated.
That's a poor argument. Maybe telegram-export could support --formatter-params column_width=480, or something like that. Having a standard way to define which parameters are valid could also help with error diagnostics, help or just listing options.
|
I am very busy in this days. May be on next week. |
Maybe it`s not very pretty code and rendering result, but it works for me.
I copied some styles from telegram desktop exporter.