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 cite_string ? #193

Closed
TNRiley opened this issue Jul 30, 2024 · 3 comments
Closed

Remove cite_string ? #193

TNRiley opened this issue Jul 30, 2024 · 3 comments
Labels
Internal Internal functionality Question Further information is requested

Comments

@TNRiley
Copy link
Collaborator

TNRiley commented Jul 30, 2024

cite_string has no real functionality and I'm still not able to envision a need for a third custom metadata field outside of some very complex use cases. In order to keep the package clean do we want to consider removing cite_string at this point?

cite_string also poses some issues with re-uploaded data. For example, when re-uploading a .csv the tables throw errors. This is probably an easy fix (I think shifting the output from unknown to NA).

I'm not sure about the work it would take to add cite_string back in later if it was determined to be a useful addition or the work/potential issues we might run into by removing at this point. Maybe just removing it from the shiny?

Thoughts?
@LukasWallrich @DrMattG @rootsandberries @kaitlynhair

@TNRiley TNRiley added Question Further information is requested Internal Internal functionality labels Jul 30, 2024
@LukasWallrich
Copy link
Collaborator

We had that discussion a couple of times already - to me, it would seem quite intuitive to want to separate databases, screening stages and search strings, but that might just be me. I would suggest at least keeping it in the R package, where most users can easily ignore it, and removing it from the Shiny app, if you feel it causes clutter and potential confusion there - but happy to go with any decision.

@LukasWallrich
Copy link
Collaborator

One discussion was here - though the original reason for removing it (an ASySD limitation) is gone: #123

@TNRiley
Copy link
Collaborator Author

TNRiley commented Aug 1, 2024

One discussion was here - though the original reason for removing it (an ASySD limitation) is gone: #123

thanks for reminding me of this discussion and for your input. Keeping it sounds like the best decision. If there are any specific items that need to be cleaned up on the shiny end of things we can add those as individual issues.

@TNRiley TNRiley closed this as completed Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Internal functionality Question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants