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

portal 4.0.0 functionality described in portal_config.md #1097

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

aartivnkt
Copy link
Contributor

Jira Ticket: PXP-xxxx

  • Remove this line if you've changed the title to (PXP-xxxx): <title>

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Copy link
Collaborator

@mfshao mfshao left a comment

Choose a reason for hiding this comment

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

would be more helpful if we can label out which field(s) are optional, and what their default values would be if omitted

@@ -481,6 +501,23 @@ Below is an example, with inline comments describing what each JSON block config
"tagSelector": {
"title": "Associated tags organized by category"
},
"studies": [ // new in data-portal 4.0.0, this configures addition of tutorial nbs to specific studies. Shown here in BRH example. For HEAL, set to []
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you elaborate a bit more about how this canbe used? with those match and fieldsToValues?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, the level of detail you are requesting is easier to communicate via code than documentation (e.g. someone needing to know more detail than the example can look at the data portal code)

Copy link
Collaborator

@mfshao mfshao Sep 20, 2022

Choose a reason for hiding this comment

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

yeah true but the code is lack of comment as well 🤦 And while we can improve documentation now, why wait?

we should try our best to not make information silos to devs who write the code

Copy link
Collaborator

@mfshao mfshao Sep 20, 2022

Choose a reason for hiding this comment

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

if you think it will be too verbose to put in this file, you can write a dedicated markdown like https://github.com/uc-cdis/data-portal/blob/master/docs/ecosystem_browser.md

Copy link
Contributor

Choose a reason for hiding this comment

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

Code doesn't always need to have comments and usually should be self documenting. Excessive documentation makes maintenance more tedious.

And while we can improve documentation now, why wait?

Because the anticipation of a BRH go live has been going on for months and the team may finally have enough momentum for it to actually happen.

we should try our best to not make information silos to devs who write the code

Putting an emphasis on writing clean, self documenting code instead of excessive standalone documentation is actually a win-win for product managers and developers: encourages product managers to look at source code and obviously makes the job more fun for a developer.

Copy link
Collaborator

@mfshao mfshao Sep 20, 2022

Choose a reason for hiding this comment

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

Excessive documentation makes maintenance more tedious.

Agreed. But I don't think the ask is excessive. If a ppl needs to go back to the original PR or code base checks among 100+ lines of changes to figure out why my portal is crashing because of missing a [] in config file or understand why the matches working from a config file doesn't work for another one, how is that code "clean and self documenting"?

encourages product managers to look at source code

Good thinking. But you cannot expect all PMs have the same level of understanding of code as devs. This is just unrealistic. And PMs and Devs are not the only person who will update portal configs to deploy a new feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey guys, I'll add some more documentation to this. May not be today, but definitely soon. I think we need a good balance of clean code and documentation, so I'll go over that and try to fill the gaps as needed. Portal is very new to me as well, so may be a good starting point to see if I can make it such that anyone can follow.

mfshao
mfshao previously requested changes Sep 20, 2022
Copy link
Collaborator

@mfshao mfshao left a comment

Choose a reason for hiding this comment

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

please address comments

@ocshawn
Copy link
Contributor

ocshawn commented May 26, 2023

i adjusted the comments a little so we can add some documentation to the code base, it would be better if it gave some idea of how to use it (code is not self documenting and has 0 comments). but since this has been sitting for 8 months i would rather get something merged, this documentation should have been part of original PR that added code

@ocshawn ocshawn requested a review from mfshao May 30, 2023 14:24
@ocshawn ocshawn dismissed mfshao’s stale review July 26, 2023 16:16

old and just want to get somthing in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants