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

Update section on 'Adding a codelist' #459

Merged
merged 4 commits into from
Oct 12, 2021

Conversation

milanwiedemann
Copy link
Member

@milanwiedemann milanwiedemann commented Oct 11, 2021

  • Add one example with version instead of date
  • Change structure of string to be more generic <codelist-id>/<tag>
  • Add screenshot illustrating where to find the info that's needed
  • Add section explaining where the csv files will be added
  • Some changes to the wording trying to be more specific

Not sure if I added the screenshot correctly and whether there are any naming conventions for the images ...

@milanwiedemann milanwiedemann linked an issue Oct 11, 2021 that may be closed by this pull request
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 11, 2021

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d76305f
Status: ✅  Deploy successful!
Preview URL: https://f783bd13.opensafely-docs.pages.dev

View logs

Copy link
Member

@iaindillingham iaindillingham left a comment

Choose a reason for hiding this comment

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

A few nitpicks, but one change: please ensure the path is codelists/codelists.txt. Thank you for the PR 🙂

Take a look at the `codelists/codelists.txt` file in the repo, and note the structure of the existing example codelists that shipped with the research template:

Your example research template doesn't include any codelists but the folder strucure and text files that are needed to include codelists already exist.
Take a look at the `codelists/codelist.txt` file in the repo, this file is currently empty but you can add one (or more) strings to this file that specify the codelists that need for your research project.
Copy link
Member

Choose a reason for hiding this comment

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

Could you change the path to codelists/codelists.txt here and below? (note the plural) And although it's a nitpick (and so you are free to ignore me!), might it be clearer to replace "strings" with "lines" both here and below? Finally, I think:

... the codelists that need for your research project.

should be:

... the codelists that you need for your research project.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree and agree, I also didn't like strings but couldn't really think of anything better, I like lines

You need to add each string into a new line of the `codelist.txt` file.
The next time you run the command `opensafely codelists update` in your terminal, the codelists you specified earlier will be added to the the `codelists/` subfolder in your project automatically so you don't need to add these files manually to your project.

For example, a `codelist.txt`: file of a project may consist of four different strings:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the first colon (:) here.

```bash
opensafely/aplastic-anaemia/2020-04-24
opensafely/asplenia/2020-06-02
opensafely/current-asthma/2020-05-06
primis-covid19-vacc-uptake/bmi_stage/v1.2
Copy link
Member

Choose a reason for hiding this comment

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

Good to give an example of a version number rather than a date 👍🏻

Copy link
Member

@iaindillingham iaindillingham left a comment

Choose a reason for hiding this comment

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

🚀

@milanwiedemann milanwiedemann merged commit 5142c26 into main Oct 12, 2021
@milanwiedemann milanwiedemann deleted the issue-454-update-add-codelist branch October 12, 2021 15:36
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.

Make clearer how to add a codelist to a project
2 participants