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

remove unused import #48

Merged
merged 5 commits into from
Jan 9, 2025
Merged

remove unused import #48

merged 5 commits into from
Jan 9, 2025

Conversation

cjyetman
Copy link
Member

js_translations never gets used in this function/script

It also doesn't appear to get used anywhere in this repo:
https://github.com/search?q=repo%3ARMI-PACTA%2Fworkflow.pacta.dashboard%20js_translations&type=code

which suggests that these could be removed also:
inst/extdata/translation/js_labels.json
inst/extdata/translation/update_translations.R

but I'm not sure if those are somehow getting picked up by pacta-dashboard-svelte

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (940f429) to head (6a9cb7d).
Report is 6 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main     #48   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         25      25           
  Lines       1208    1205    -3     
=====================================
+ Misses      1208    1205    -3     

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

Copy link

github-actions bot commented Dec 12, 2024

Docker build status

commit_time git_sha image
2025-01-09T14:14:12Z 6a9cb7d ghcr.io/rmi-pacta/workflow.pacta.dashboard:main

Copy link

github-actions bot commented Dec 12, 2024

PR Preview Action v1.5.0
Preview removed because the pull request was closed.
2025-01-09 14:24 UTC

@jdhoffa
Copy link
Member

jdhoffa commented Dec 12, 2024

which suggests that these could be removed also: inst/extdata/translation/js_labels.json inst/extdata/translation/update_translations.R

but I'm not sure if those are somehow getting picked up by pacta-dashboard-svelte

Searching the pacta-dashboard-svelte repo suggests that these are not used at all, however I would leave it to @MonikaFu to indicate, so we are absolutely sure.

AlexAxthelm
AlexAxthelm previously approved these changes Dec 12, 2024
@cjyetman
Copy link
Member Author

Putting in draft mode until @MonikaFu confirms

@cjyetman cjyetman marked this pull request as draft December 12, 2024 16:29
@MonikaFu
Copy link
Contributor

MonikaFu commented Jan 7, 2025

@cjyetman it is correct that we don't need this data in the dashboard. In the interactive report it contains a bunch of labels for different plots that could then we translated to different languages based on another dataset containing translations. For the dashboard I decided to hardcode the labels as part of the plots for now as this simplified the implementation and there were no requirements to display the dashboard in any other language than English.

@cjyetman
Copy link
Member Author

cjyetman commented Jan 7, 2025

Like I said, I'm not sure if those are somehow getting picked up by pacta-dashboard-svelte (or elsewhere)... but if they are, they should probably be moved over there because one repo having a dependency on an otherwise orphaned file in another repo is too easy to get mixed up.

@cjyetman
Copy link
Member Author

cjyetman commented Jan 7, 2025

Alternatively, we could maybe document what these files are for in this repo so it's clear?

@MonikaFu
Copy link
Contributor

MonikaFu commented Jan 8, 2025

@cjyetman they are not used in pacta-dashboard-svelte and as far as I know this is the only place they could be used. So I think it should be safe to remove. Not sure how best to test if this is indeed the case.

@cjyetman
Copy link
Member Author

cjyetman commented Jan 8, 2025

@cjyetman they are not used in pacta-dashboard-svelte and as far as I know this is the only place they could be used. So I think it should be safe to remove. Not sure how best to test if this is indeed the case.

Maybe @jdhoffa has an idea of how to disentangle this?

@jdhoffa
Copy link
Member

jdhoffa commented Jan 8, 2025

Remove them all

@cjyetman
Copy link
Member Author

cjyetman commented Jan 8, 2025

Remove them all

also deleted data files per @jdhoffa, opening for review

@cjyetman cjyetman marked this pull request as ready for review January 8, 2025 16:53
@cjyetman cjyetman merged commit 2d4bf64 into main Jan 9, 2025
31 checks passed
@cjyetman cjyetman deleted the remove-js-translation-import branch January 9, 2025 16:18
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.

4 participants