-
Notifications
You must be signed in to change notification settings - Fork 440
Introduce cache database and reindex command #4650
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
base: master
Are you sure you want to change the base?
Conversation
ba4dcf6 to
110a261
Compare
bruntib
left a comment
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.
Some tests would also be useful.
|
|
||
|
|
||
| def __process_file(file_path: str) -> Tuple[str, List[str]]: | ||
| with open(file_path, 'rb') as fp: |
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 open(file_path, 'rb') as fp: | |
| with open(file_path, 'rb', encoding='utf-8', errors='ignore') as fp: |
Conventionally we're using utf-8 encoding for every open(). We should get rid of this in the future by setting encoding globally.
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.
We are opening the file in binary mode, where encoding and errors options shouldn't be specified, see docs: https://docs.python.org/3/library/functions.html#open
For XML parsing, the plistparser uses lxml, see (tools/report-converter/codechecker_report_converter/report/parser/plist.py, line 104). It accepts the data in binary form, thus we are reading the file in binary mode.
|
|
||
| def __create_tables(self): | ||
| if not self.__table_exists("plist_lookup"): | ||
| self.__cur.execute("CREATE TABLE plist_lookup(plist, source)") |
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.
Shouldn't column types be indicated here? The default type is BLOB (https://www.sqlite.org/datatype3.html: 3.1)
| plist_files = list(filter(lambda f: f.endswith( | ||
| plistparser.EXTENSION), os.listdir(report_dir))) | ||
| plist_files = list(map(lambda f: os.path.abspath( | ||
| os.path.join(report_dir, f)), plist_files)) | ||
| plist_files = list(filter(lambda f: f not in indexed_files, plist_files)) |
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.
List generation is not necessary. I think, the generator objects could be nested into each other.
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.
Changed in commit 6b7cce9
110a261 to
6b7cce9
Compare
Thanks for the review! I will add tests in a separate commit. |
6b7cce9 to
c5467c8
Compare
c5467c8 to
f05de3e
Compare
|
@bruntib @Discookie |
No description provided.