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

Implement sorting of events via config file #81

Merged
merged 30 commits into from
Jun 12, 2024
Merged

Conversation

podliashanyk
Copy link
Contributor

@podliashanyk podliashanyk commented Mar 13, 2024

Related discussion #96

Depends on Uninett/zinolib#61

Implemented changes:

  • Added all sorting methods that exist in Ritz TK
  • Added a default sorting (the unchanged order in which server sends events)
  • Added a definition for a default sorting used in curitz and by SSC (called lasttrans)
  • Sorting method can only be changed via config file
  • Document configurable sorting methods
  • Made NTIE work with sorting (using SimpleCache from flask-caching)

Attention needed:

  • to the implementation itself. Are there redundancies, smelly code etc?
  • should sorting methods definitions and methods be moved somewhere else from endpoints.py?
  • I have tested whether order of events matches with the ones in Ritz TK/Curitz. The original sorting methods in TK are not documented, so I had to reverse engineer them and guess what set of parameters each sorting method goes after. So a test by another person would be nice.
  • do you have better names? I have followed the original naming for methods so far.

Further work:

  • Make it possible to change sorting method via web UI (not a part of original implementation in Curitz)

@podliashanyk podliashanyk added enhancement New feature or request help wanted Extra attention is needed beta Part of beta release labels Mar 13, 2024
@podliashanyk podliashanyk self-assigned this Mar 13, 2024
Copy link

github-actions bot commented Mar 13, 2024

Test results

    4 files      4 suites   9s ⏱️
  38 tests   38 ✔️ 0 💤 0
152 runs  152 ✔️ 0 💤 0

Results for commit 2ba497e.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 63.80952% with 38 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@1cbd8b9). Learn more about missing BASE report.

Files Patch % Lines
src/howitz/endpoints.py 60.00% 36 Missing ⚠️
src/howitz/__init__.py 77.77% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #81   +/-   ##
=======================================
  Coverage        ?   48.60%           
=======================================
  Files           ?       16           
  Lines           ?     1004           
  Branches        ?        0           
=======================================
  Hits            ?      488           
  Misses          ?      516           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@podliashanyk podliashanyk mentioned this pull request Mar 13, 2024
2 tasks
README.rst Outdated Show resolved Hide resolved
src/howitz/endpoints.py Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
podliashanyk and others added 7 commits April 4, 2024 09:19
Bad code and not all sorting results are equivalent to sorting results in nowadays Zino TK
All sorting methods are implemented and work as intended
Sorting method is configured as sort_by (str) under [howitz] section.
double underscore in _value_ holds some meaning for Enum, but
the other ones seem unecessary
In order to avoid circular imports
@podliashanyk podliashanyk added discussion paused Assignee is busy with things of higher priority labels Jun 6, 2024
Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

Will rereview after rebase.

NTS: tests, the big if

@podliashanyk podliashanyk removed the paused Assignee is busy with things of higher priority label Jun 10, 2024
Otherwise, the logger.debug statement in sorted_events() fails with RuntimeError
@podliashanyk podliashanyk added blocked Depends on other issue being solved and removed blocked Depends on other issue being solved labels Jun 10, 2024
@podliashanyk podliashanyk requested a review from hmpf June 11, 2024 13:03
src/howitz/config/models.py Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
src/howitz/endpoints.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
src/howitz/__init__.py Outdated Show resolved Hide resolved
@podliashanyk podliashanyk requested a review from hmpf June 12, 2024 12:34
howitz.toml.example Outdated Show resolved Hide resolved
@hmpf hmpf self-requested a review June 12, 2024 12:50
@podliashanyk podliashanyk merged commit 311a477 into main Jun 12, 2024
5 checks passed
@podliashanyk podliashanyk deleted the events-sorting branch June 12, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Part of beta release discussion enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants