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

Expand adaptor docs #260

Merged
merged 8 commits into from
Jul 21, 2023
Merged

Expand adaptor docs #260

merged 8 commits into from
Jul 21, 2023

Conversation

amberrignell
Copy link
Contributor

Summary

Add a high-level, single-sentence summary of what this PR changes.

Details

Add technical details of what you've changed (and why).

Issues

Reference any related issues here.

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, has the migration tool been run and the
    migration guide followed?
  • Are there any unit tests? Should there be?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.

Copy link
Member

@taylordowns2000 taylordowns2000 left a comment

Choose a reason for hiding this comment

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

hey was the line break on line 65 intentional? please remove if not. (there still be something going on with your formatter and the prettierrc configuration.)

@amberrignell amberrignell changed the title update copy folder command Expand adaptor docs Jun 9, 2023
Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

I've made a couple of comments.

It would be really nice to have a monorepo tool to initialise a new adaptor using the template. It could do most of the monkeywork of coping files around and swapping out adaptor names. That would simplify a lot of this stuff!

README.md Outdated

The configuration schema defines required parameters and their expected values
- Update the top-level changelog comment `# @openfn/language-template` to your adaptor name and delete the example content. Delete ast.json, then run `pnpm clean` (this will remove the dist, types and docs folders).
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably still want the language- prefix so can we say:

Update the top-level changelog comment `# @openfn/language-template` to `# @openfn/language-youradaptorname`

README.md Outdated Show resolved Hide resolved
README.md Outdated

#### 2. Create a job with the operation you'd like to add, and run it so it fails

- Create a _temporary_ folder where you can test your operation manually by running a job: `mkdir tmp`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't emphasis temporary, it makes it look like you have to do something special. Like, how do I create a temporary folder?

Probably important to note that the contents of tmp don't get checked in to github - so this folder name isn't arbitrary, it's a convention we use.

README.md Outdated
- The job should fail - we haven't build the adaptor yet! Let's go do that

#### 3. Create your adaptor function
- Head to `lib/Adaptor.js`, and define your first function.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

src

@josephjclark
Copy link
Collaborator

I am merging this in for today's release. It's an improvement on the current docs and we can raise issues/PRs later if we want to make further changes.

Thanks @amberrignell !!

@josephjclark josephjclark merged commit 09d17fd into main Jul 21, 2023
1 check passed
@josephjclark josephjclark deleted the contributing branch July 21, 2023 10:40
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.

3 participants