chore: using f-string instead of string.format #428
Conversation
dungdm93
requested review from
allisonsuarez,
dikshathakur3119,
feng-tao and
jinhyukchang
as code owners
Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>
|
|
||
| self.sql_stmt = AthenaMetadataExtractor.SQL_STATEMENT.format( | ||
| where_clause_suffix=conf.get_string(AthenaMetadataExtractor.WHERE_CLAUSE_SUFFIX_KEY), | ||
| catalog_source=self._cluster | ||
| ) | ||
|
|
||
| LOGGER.info('SQL for Athena metadata: {}'.format(self.sql_stmt)) | ||
| LOGGER.info('SQL for Athena metadata: %s', self.sql_stmt) |
There was a problem hiding this comment.
I believe we should be using f here as well for consistency
There was a problem hiding this comment.
@allisonsuarez string.format or f-string is eager evaluation, which mean that the logging message still evaluate event it's not logged to output.
On other hand, LOGGER.info(format, args) is lazy evaluation. So in somehow, it's better.
What do you think?
| @@ -71,7 +71,7 @@ def init(self, conf: ConfigTree) -> None: | |||
|
|
|||
| self.sql_stmt = sql_alch_conf.get_string(SQLAlchemyExtractor.EXTRACT_SQL) | |||
|
|
|||
| LOGGER.info('SQL for Db2 metadata: {}'.format(self.sql_stmt)) | |||
| LOGGER.info('SQL for Db2 metadata: %s', self.sql_stmt) | |||
There was a problem hiding this comment.
same here, should use f as well
| @@ -47,7 +47,7 @@ def _execute_query(self) -> Iterable[Any]: | |||
| Use cursor to execute the {sql} | |||
| :return: | |||
| """ | |||
| LOGGER.info('Executing query: \n{}'.format(self.sql)) | |||
| LOGGER.info('Executing query: \n%s', self.sql) | |||
There was a problem hiding this comment.
oh wait is this functionality you are leveraging from LOGGER.info?
There was a problem hiding this comment.
Yep. It's API from LOGGER.info
|
|
||
| from databuilder import Scoped | ||
| from databuilder.extractor.base_extractor import Extractor | ||
| from databuilder.extractor.sql_alchemy_extractor import SQLAlchemyExtractor | ||
| from databuilder.models.table_metadata import TableMetadata, ColumnMetadata | ||
| from itertools import groupby | ||
|
|
||
| from pyhocon import ConfigFactory, ConfigTree |
There was a problem hiding this comment.
typical python lib order based on https://www.python.org/dev/peps/pep-0008/
Imports should be grouped in the following order:
Standard library imports.
Related third party imports.
Local application/library specific imports.
Even we don't have CI to enforce it, it would good to still use this model (e.g databuilder import should come last) WDYT?
There was a problem hiding this comment.
@feng-tao Do you mind if I setup a CI to check import order?
There was a problem hiding this comment.
that will be great! thanks @dungdm93
Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>
There was a problem hiding this comment.
lgtm, will let @allisonsuarez take another look befor emerge, thanks @dungdm93 !
Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>
My only concern was with the logger.info changes, if that a part fo the function and doesn't require and f string I think this is good to go :) thanks @dungdm93 ! |
|
@dungdm93 are you planning to add the github CI to check import order in a different pr? |
|
@feng-tao GitHub CI run |
|
@dungdm93 miss that part, lgtm then. |
Signed-off-by: Đặng Minh Dũng dungdm93@live.com
Summary of Changes
Currently, our repo is
string.formatheavy-using. Some of them are unnecessary like:amundsendatabuilder/databuilder/extractor/athena_metadata_extractor.py
Line 48 in 453a18b
This PR is all about clean that up and make the code more concise by using f-string instead of string.format
Tests
No tests change. Already pass
make testDocumentation
N/A
CheckList
Make sure you have checked all steps below to ensure a timely review.
make test