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

Add a feature for exporting method invocation list #61

Merged
merged 26 commits into from
Apr 26, 2023

Conversation

cheesecat47
Copy link
Contributor

Hello, I was impressed with the scavenger presentation at Deview 2023. So I forked this repository as I could use it for something I'm working on.
It's been helpful for me to identify all the methods in my Java project in a short time and know if they are executing or not.
However, in the current version, there was no way to export the collected information on a website (although I can query it directly from the DB). I wanted to contribute, so I added that feature.
This is my first time contributing to open source project, so it may need to be improved on many points. Any feedback would be greatly appreciated.

Here's the description of this PR.

  • Added an "Export" button to the bottom of the SnapshotDetail page.
    Screenshot 2023-04-10 at 4 26 02 PM
  • Press that button to download a TSV file with the following information.
    Screenshot 2023-04-10 at 4 27 01 PM

@cheesecat47 cheesecat47 requested review from a team and junoyoon as code owners April 10, 2023 07:42
@github-actions
Copy link

github-actions bot commented Apr 10, 2023

Scavenger Test Results

154 files  154 suites   34s ⏱️
254 tests 242 ✔️ 12 💤 0
256 runs  244 ✔️ 12 💤 0

Results for commit a9fd260.

♻️ This comment has been updated with latest results.

@taeyeon-Kim
Copy link
Contributor

@cheesecat47 Firstly, thank you so much for your interest in scavenger :)
I'd like to know what the primary purpose of this feature is.
If it's to determine which methods are considered dead code (not invoked), I'd suggest adding a filter button next to the search bar to see only not invoked methods.
I think this would be a cool feature if it could be used in more ways than just that.

@cheesecat47
Copy link
Contributor Author

@taeyeon-Kim I appreciate your feedback!
I wanted to add this feature to compare the results of Scavenger with the results of the tool I'm using. Also, because I'm more familiar with Python, I wanted to export Scavenger results to a CSV file. However, in the current version of Scavenger, there is no way to download the information displayed on the website. That's why I tried it.
Is the reason the export button wasn't added because exporting results to a file wasn't in the use cases you expected? If so, replacing the button with the filter feature you mentioned would be better. It seems more useful to show multiple search results than I did, like showing query results in Kibana.
If it's okay to keep the button, then it would be nice to modify it to export the filtered results (the screen you're currently looking at) so you don't have to create a contextual button.

@taeyeon-Kim
Copy link
Contributor

taeyeon-Kim commented Apr 14, 2023

@cheesecat47
Sorry for the late comment.
First, how about moving the feature to the snapshot table page?
image

<!-- main.js -->
import {faFileExport} from "@fortawesome/free-solid-svg-icons";
iconRegister.add(faFileExport);


 <!--  SnapshotTable.vue-->
<el-table-column :label="'$t('message.snapshot.export')'" width="80" align="center">
    <template #default="scope">
        <el-button circle type="primary" @click="refreshSnapshot(scope.row.id)">
            <font-awesome-icon icon="fa-solid fa-file-export"/>
        </el-button>
    </template>
</el-table-column>

I would also like to see this feature changed to allow exporting per snapshot instead of for the entire invocation.

@cheesecat47
Copy link
Contributor Author

@taeyeon-Kim First, thank you for your detailed and kind comments.
May I ask how did you use features like live reloading in your development? Because building every time takes too long..

Copy link
Collaborator

@junoyoon junoyoon left a comment

Choose a reason for hiding this comment

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

@cheesecat47
I left last reviews. please check it.

Related PR naver#61

Move exportSnapshotMethod function to SnapshotController.
And also modified namings from ExportSnapshotSth to SnapshotExportSth to follow the convention.
Related PR naver#61

Fix the name of SnapshotExportEntity to DbRow because there's no SnapshotExport table in DB.
Related PR naver#61

Modify exportSnapshot() api url from .../export/snapshot/id to .../snapshot/id/export, simillar to refreshSnapshot().
Related PR naver#61

Move SnapshotExportDbRow from scavenger-entity to scavenger-api
@@ -105,7 +105,7 @@ class SnapshotController(
)
}

@GetMapping("/customers/{customerId}/export/snapshot/{snapshotId}", produces = ["text/csv"])
@GetMapping("/customers/{customerId}/snapshot/{snapshotId}/export", produces = ["text/csv"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

very good

@junoyoon
Copy link
Collaborator

@cheesecat47 good enough.!!

Copy link
Collaborator

@junoyoon junoyoon left a comment

Choose a reason for hiding this comment

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

Thx.. Approved.

Merge

@junoyoon junoyoon merged commit 6b5e4db into naver:develop Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
Related PR #61

Fix types and nullables according to schema.sql.
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
Related PR #61

Remove if statement for checking `err.response.status` code.
And also remove the `err` variable like `editSnapshot` function of SnapshotForm.vue, because it seems that there is no usage anymore.
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
Related PR #61

Fix messages of `message.export` to make it consistent.
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
Related PR #61

Move exportSnapshotMethod function to SnapshotController.
And also modified namings from ExportSnapshotSth to SnapshotExportSth to follow the convention.
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
Related PR #61

Fix the name of SnapshotExportEntity to DbRow because there's no SnapshotExport table in DB.
junoyoon pushed a commit that referenced this pull request Apr 26, 2023
Related PR #61

Modify exportSnapshot() api url from .../export/snapshot/id to .../snapshot/id/export, simillar to refreshSnapshot().
@sohyun-ku sohyun-ku mentioned this pull request Jan 31, 2024
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.

5 participants