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

hotfix for the date formatting issue #93

Merged
merged 2 commits into from
Sep 10, 2024
Merged

hotfix for the date formatting issue #93

merged 2 commits into from
Sep 10, 2024

Conversation

pgulley
Copy link
Member

@pgulley pgulley commented Sep 10, 2024

Kind of breaks the abstraction pattern, but I think it's fine for this case- dates just need a separate formatter in the es_client. Validated in a dev stack.

@pgulley pgulley requested a review from philbudne September 10, 2024 19:37
@philbudne
Copy link
Contributor

In the dark here, 'cause I don't know much about the code, or the bug, and I know this is code that came from IA, and this is marked "hotfix", so I imagine there is some urgency.... but:

  1. There another invocation of format_counts (in get_terms) does this apply there?
  2. To me, it's hard to see the difference between the two branches of the "if", and it might be more compact, and clearer if there was one function, that took the aggregation type (either as a string or as a bool) and returned the right thing.
  3. my_dict.update({key: ....}) is less clear to me than my_dict[key] = ... but maybe that's just me?????

@pgulley
Copy link
Member Author

pgulley commented Sep 10, 2024

In the dark here, 'cause I don't know much about the code, or the bug, and I know this is code that came from IA, and this is marked "hotfix", so I imagine there is some urgency.... but:

1. There another invocation of `format_counts` (in `get_terms`) does this apply there?

2. To me, it's hard to see the difference between the two branches of the "if", and it might be more compact, and clearer if there was one function, that took the aggregation type (either as a string or as a bool) and returned the right thing.

3. `my_dict.update({key: ....})` is less clear to me than `my_dict[key] = ...` but maybe that's just me?????

Agree about the functional formatting- simplified per those notes!
The other instance in the top_terms path shouldn't apply here!

@pgulley pgulley merged commit 7ca6044 into main Sep 10, 2024
1 check passed
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.

2 participants