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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor README.md #61

Merged
merged 2 commits into from May 31, 2022
Merged

refactor README.md #61

merged 2 commits into from May 31, 2022

Conversation

mixmix
Copy link
Member

@mixmix mixmix commented May 26, 2022

Motivation / Approach

for this : I wanted to read this in a way which was helpful for beginners. Taking from teaching, the idea I applied was "give me just enough detail to get started, then come back around with more detail as required".

So we start out very simple, then put more detail in the API section.

馃敟 I also mutated the API to make things clearer

As I was updating this I stumbled across things which felt like they made telling the story harder.
Things like:

  • API behaves differently for the root feed than for all other feeds. This means we have to write more things
  • use of words which conflict with other modules in ecosystem: feedtype is actually more like BFE format

I've commented those with 馃敟 so we can discuss.

Andre I could have split that out into a different PR but I was writing from the point of view of ideal end state. Lets decide what we want then mutate this PR to end up where we want to be

@mixmix
Copy link
Member Author

mixmix commented May 26, 2022

README.md Outdated Show resolved Hide resolved

## API

### `sbot.metafeeds.findOrCreate(cb)`
Copy link
Member Author

@mixmix mixmix May 26, 2022

Choose a reason for hiding this comment

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

wrote and explicit signature documentation for this special usage to make it clear.
I think this should be moved down in the docs

馃敟
I disliked that the object you got back from this call of this method was different, so I made it isomorphic:

  • metafeed: null
  • feedpuprpose: 'root'
  • feedtype: 'bendybutt-v1',
  • metadata: {}

NOTE I think we should change feedtype to format

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in #62

README.md Show resolved Hide resolved
{ feedpurpose, feedformat, metafeed, metadata }
```

NOTE - may include `seed`, `keys` if this is one of your feeds.
Copy link
Member Author

Choose a reason for hiding this comment

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

馃敟 is this true?
This current signature really suggests you can use this on other peoples feedIds too....

README.md Outdated Show resolved Hide resolved
@mixmix mixmix mentioned this pull request May 26, 2022
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mixmix
Copy link
Member Author

mixmix commented May 30, 2022

@staltz have addressed all comments. thanks for review

Do you want to re-review, or shall I merge?

@mixmix
Copy link
Member Author

mixmix commented May 30, 2022

lol... changes README, tests fail D:

@staltz
Copy link
Member

staltz commented May 31, 2022

have addressed all comments. thanks for review

Do you want to re-review, or shall I merge?

@mixmix Let's merge, but I'll make a follow-up PR to recover some of the text you deleted, like under the "Example usage" section, there was quite a good amount of explanation and hand-holding that shouldn't be deleted.

As a general rule that arj and I have been following, the PR author merges only after the PR reviewer has approved with a green checkmark. Any moment before that is considered "don't merge yet".

@mixmix mixmix merged commit e49df42 into master May 31, 2022
@staltz staltz deleted the README_tweaks branch May 31, 2022 12:30
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.

None yet

2 participants