-
Notifications
You must be signed in to change notification settings - Fork 64
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
Usage Notes: Document format of profiles.json #626
Usage Notes: Document format of profiles.json #626
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no issues with the documentation, LGTM!:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just some small comments
docs/usage-notes.md
Outdated
|
||
`profileName` refers to the profile name displayed in the session select page. `encodedText` refers to the repository which stores the required settings for your custom session. | ||
|
||
> **Note**: You **must** have both of these fields in each `Profile` and no extra unnecessary fields! Else, the `profile.json` file that you have supplied will not be parsed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently not checking for unnecessary fields, should we add that functionality in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah alright, I think we can leave it out for now!
When we actually check for unnecessary fields, then one of us can update the dev notes whenever necessary!
docs/usage-notes.md
Outdated
|
||
`profileName` refers to the profile name displayed in the session select page. `encodedText` refers to the repository which stores the required settings for your custom session. | ||
|
||
> **Note**: You **must** have both of these fields in each `Profile` and no extra unnecessary fields! Else, the `profile.json` file that you have supplied will not be parsed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll be good to also state explicitly "no empty string" for field values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Codecov Report
@@ Coverage Diff @@
## master #626 +/- ##
==========================================
- Coverage 67.59% 67.50% -0.10%
==========================================
Files 72 72
Lines 2160 2160
Branches 198 198
==========================================
- Hits 1460 1458 -2
Misses 660 660
- Partials 40 42 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some brief comments
Co-authored-by: Anubhav <35621759+anubh-v@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good! Just some minor queries. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
@CATcher-org/2021-devs any further comments by anyone?
Summary
This PR fixes #584, which adds a section on the format of
profiles.json
to be supplied.Description
You can see the actual visual changes here
Proposed Commit Message
Dev Notes: Add section on E2E tests