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.

Sign up for GitHub

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

Jump to bottom

Draft Explainer #309

Merged
merged 29 commits into from
May 30, 2022
Merged

Draft Explainer #309

merged 29 commits into from
May 30, 2022

Conversation

mmccool
Copy link
Contributor

@mmccool mmccool commented May 13, 2022 •
edited
Loading

Ready for review. Please check carefully for technical accuracy. We should also decide whether we should really allow non-HTTP self-description explorations given that our spec has almost no assertions constraining them. The explainer would have to be updated if we do decide to support non-HTTP explorations (which the current specification DOES allow).

@mmccool
Copy link
Contributor Author

mmccool commented May 16, 2022

Comments from mtg May 16:

  • Can include LinkSmart, it's not up to date with the latest spec but can be considered a partial implementation. We do have test results for it.
  • Opening of Arch should be under "requirements". Also, we should dig up original requirements, and what's listed in Arch, to be consistent.

JKRhb
JKRhb reviewed May 16, 2022
View reviewed changes
Copy link
Member

@JKRhb JKRhb left a comment

Choose a reason for hiding this comment

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

Looks good to me in general :) See a couple of comments below.

explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
@mmccool mmccool changed the title WIP: Draft Explainer Draft Explainer May 17, 2022
@mmccool mmccool mentioned this pull request May 19, 2022
Closed
@mmccool mmccool marked this pull request as ready for review May 19, 2022 13:42
farshidtz
farshidtz suggested changes May 25, 2022
View reviewed changes
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

@mmccool That's a great explanainer. Well done!

I've found some typos and minor issues worth fixing.

explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
relu91
relu91 approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Left some minor comments below, but overall the explainer looks good!

explainer/Explainer.md Show resolved Hide resolved
explainer/Explainer.md Outdated Show resolved Hide resolved
explainer/Explainer.md Show resolved Hide resolved
mmccool and others added 4 commits May 30, 2022 10:45
@mmccool @farshidtz
Update explainer/Explainer.md
ba4f855 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
8f7c10d 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
7a9cdeb 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool
explorations != TDD
60ee012
mmccool and others added 11 commits May 30, 2022 10:53
@mmccool @farshidtz
Update explainer/Explainer.md
06a6515 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
73d2141 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
f586561 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
f5f0d5b 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool
address review comments
d842b40
@mmccool
Merge branch 'explainer' of github.com:mmccool/wot-discovery into exp…
0c9b862 
…lainer
@mmccool @farshidtz
Update explainer/Explainer.md
ee29762 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
948bd00 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
2d6bbff 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
5fa5b4d 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@mmccool @farshidtz
Update explainer/Explainer.md
1f61470 
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@farshidtz farshidtz mentioned this pull request May 30, 2022
Open
@mmccool mmccool merged commit a644379 into w3c:main May 30, 2022
@farshidtz farshidtz mentioned this pull request Jun 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JKRhb JKRhb JKRhb left review comments

@danielpeintner danielpeintner danielpeintner left review comments

@farshidtz farshidtz farshidtz requested changes

@k-toumura k-toumura k-toumura approved these changes

@relu91 relu91 relu91 approved these changes

@AndreaCimminoArriaga AndreaCimminoArriaga Awaiting requested review from AndreaCimminoArriaga

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mmccool @relu91 @farshidtz @danielpeintner @JKRhb @k-toumura