Skip to content

Timechart refactoring - #490

Open
51-code wants to merge 5 commits into
teragrep:mainfrom
51-code:timechart-refactor
Open

Timechart refactoring#490
51-code wants to merge 5 commits into
teragrep:mainfrom
51-code:timechart-refactor

Conversation

@51-code

@51-code 51-code commented Jan 29, 2025

Copy link
Copy Markdown
Contributor

Fixes #473 .

Refactored TimechartStep:

  • Made it immutable
  • Removed the AbstractTimechartStep class which basically just contained getters and setters and the class variables

Reworked TimechartTest:

  • The tests broke because they checked the correct translation of the parse tree with the getters from TimechartStep. Reworked the tests to assert equality with an expected TimechartStep object instead.

Refactored TimechartTransformation:

  • Removed unused parameters, unused code, commented out code and so on.
  • Refactored the visiting logic. Old version used a for loop to go over the child nodes and collect values for the TimechartStep. The new version calls visitChildren() and collects the values for TimechartStep to class variables instead.

@51-code 51-code added the feature Existing feature label Jan 29, 2025
@51-code 51-code self-assigned this Jan 29, 2025
@51-code
51-code requested a review from eemhu January 29, 2025 11:21
@eemhu

eemhu commented Jan 29, 2025

Copy link
Copy Markdown
Contributor

Check the possibility of using ContextValue to avoid object mutability.

@51-code

51-code commented Feb 24, 2025

Copy link
Copy Markdown
Contributor Author

Refactored the Transformation class to be immutable with span and divByInst variables using the ContextValue object.

Refactored parsing span quite a lot as it was heavily dependent on utility functions. Created new objects, SpanParameter and TimeRange, because of it.

AggregateFunctions are still mutable as it's impossible to make it immutable without grammar changes to the renaming of the columns first and there's an open PR about that in PTH-03: # 94

@eemhu eemhu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the new objects introduced could use some unit tests, otherwise looks good

@51-code

51-code commented Feb 25, 2025

Copy link
Copy Markdown
Contributor Author

I think the new objects introduced could use some unit tests, otherwise looks good

Created unit tests for SpanParameter and TimeRange. The contextValue classes are very hard to test because of the ANTLR parse trees involved. The ANTLR parsing is covered by the translation tests anyways, so I think creating tests for just the two objects is sufficient.

@51-code
51-code requested a review from eemhu February 25, 2025 11:14
@51-code
51-code requested a review from kortemik February 25, 2025 11:28
@51-code 51-code added the review label Feb 25, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tests are missing an essential test that the timechart works end to end as-in the dataset is aggregated properly. please add one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Existing feature review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor timechart command

3 participants