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.

Sign up for GitHub

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

Jump to bottom

Handle public and private CSVs #218

Merged
merged 29 commits into from
Jan 16, 2025
Merged

Handle public and private CSVs #218

merged 29 commits into from
Jan 16, 2025

Conversation

mccalluc
Copy link
Contributor

@mccalluc mccalluc commented Jan 13, 2025 •
edited
Loading

For the reviewer:

  • Does the way the choices are explained in the UI make sense? Coming out of the design meeting, there was feedback that that needed to be improved.
  • Some manual testing would be good.
  • Will this codebase be maintainable? If this is getting scary, is there anything that should change? Do you think the complexity would be comparable in any other framework?

mccalluc added 29 commits January 13, 2025 15:21
@mccalluc
handle public and private CSV in CLI... but nothing downstream
afc07fc
@mccalluc
coverage
f4ec7a3
@mccalluc
More readable CLI help
492c4d6
@mccalluc
fake data into separate function
5858129
@mccalluc
warn -> error
2b95650
@mccalluc
warn -> error
25083dd
@mccalluc
add private and public component params
c76ac79
@mccalluc
add explanation in UI
9a2ecc3
@mccalluc
add cards to organize first tab
7f6cc46
@mccalluc
stub where the warning message will go
b900e05
@mccalluc
warning message about column mismatch
c398c9a
@mccalluc
better formating on list
64c01a3
@mccalluc
linting
327395d
@mccalluc
make the "Define analysis" button conditional
9c2439d
@mccalluc
fix label in end-to-end
3a0de4e
@mccalluc
reformat for readability
c280b0b
@mccalluc
match -> mismatch
43ef859
@mccalluc
read either public or private
5bbcc91
@mccalluc
move out content of simulation card
9712374
@mccalluc
Different simulation card if public CSV
0893d72
@mccalluc
fix renaming bugs
630d5a6
@mccalluc
add test to fix coverage; use "Optional"
cb394fb
@mccalluc
factor mock data generation out of make_accuracy_histogram
3f4b11b
@mccalluc
public and private previews
2cbc826
@mccalluc
start testing conditional display for public vs private
38160f5
@mccalluc
nb reads public or private
48c1239
@mccalluc
also make plot title conditional
6f25df7
@mccalluc
factor out shared descriptions
a4ca970
@mccalluc
missing f on f-string
df1a17d
@mccalluc mccalluc marked this pull request as ready for review January 15, 2025 16:03
@ekraffmiller
Copy link
Member

@mccalluc I think that there are some inherent challenges with Shiny, that are addressed very well in WHAT-WE_LEARNED.md. I don't know if using another framework would reduce the complexity, but it might make the complexity more manageable.
Part of the reason we decided on Shiny is that we are hoping using Python will encourage community support and contributions. Is there any way we can get some feedback to test that theory? Maybe from OpenDP interns?

ekraffmiller
ekraffmiller approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@ekraffmiller ekraffmiller left a comment

Choose a reason for hiding this comment

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

The wording of the explanation looks good, and I tried it with mismatching files, and saw appropriate error messages.

@mccalluc mccalluc merged commit 73c04b1 into main Jan 16, 2025
2 checks passed
@mccalluc mccalluc deleted the 217-public-and-private-csv branch January 16, 2025 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekraffmiller ekraffmiller ekraffmiller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
DP Wizard
Status: Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Public and private CSV
2 participants
@mccalluc @ekraffmiller