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

Get virtual table #63

Merged

Conversation

kavehshahedi
Copy link
Contributor

@kavehshahedi kavehshahedi commented May 28, 2024

Implementation of the get-virtual-table-lines in the tsp-python-client. The endpoint is already present in the tsp-server.

Get Virtual Table Header (i.e., columns)

This implementation gets the table column information from the TSP server.

python3 tsp_cli_client --get-virtual-table-columns OUTPUT_ID --uuid UUID

Get Virtual Table Lines

To fetch the table data from the TSP server, we may use either --table-line-index (as the start index row) or --table-times (as the times of the rows).

python3 tsp_cli_client --get-virtual-table-lines OUTPUT_ID --uuid UUID --table-line-index INDEX --table-line-count COUNT --table-column-ids IDs --table-search-direction DIRECTION --table-search-expression COLUMN_ID EXPRESSION

Misc

The info regarding both functionalities has been included in the readme file.

@kavehshahedi kavehshahedi marked this pull request as ready for review June 24, 2024 17:55
@kavehshahedi kavehshahedi changed the title Get virtual table lines Get virtual table Jun 25, 2024
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

It looks good. Small comment on the code.

Please do a git rebase the change on the latest in master. No merge commit.

Also, there should be some minimal unit test added. Please note when running the unit test, it will WIPE-OUT your local Trace Compass server workspace. So, backup your workspace before running any unit tests. The workspace and be found ~/.tracecompass-webapp.

There is warning in the README but often it gets unnoticed:

**Note: Executing the tests will delete all traces and experiments from trace server workspace. Backup workspace before running the tests or start from fresh workspace.**

tsp/virtual_table_model.py Show resolved Hide resolved
print(" cells:")
for i, cell in enumerate(self.cells):
cell.print()
print(f" {'-' * 10}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also tags when the search_filter_expression is set to indicate that there is a match. tags is bitmask and right now we only support one non-null value if the cell matches. Maybe should print matched or not matched in case of searching. tags is an optional field and might not be present.

See here https://github.com/eclipse-tracecompass/org.eclipse.tracecompass/blob/d365dfc9a9d4305191387af7f8d078958e74d030/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/internal/provisional/tmf/core/model/events/TmfEventTableDataProvider.java#L569

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So based on my understanding, if tags==8 , then I should return matched? Or should I just return the tags itself?
Because based on the docs, it seems that 0 is none, 1/2 are reserved, 4 is border and 8 is highlight.

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

My previous review comments have not been addressed in the latest update.

Also, please

  • rebase to the latest in master branch (there have been commits).
  • add some unit test
  • don't use merge commits

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the contribution.

@MatthewKhouzam MatthewKhouzam self-requested a review July 18, 2024 22:14
@MatthewKhouzam MatthewKhouzam merged commit 9bb682a into eclipse-cdt-cloud:master Jul 19, 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.

None yet

3 participants