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

Minor fixes #10

Merged
merged 9 commits into from
Dec 17, 2019
Merged

Minor fixes #10

merged 9 commits into from
Dec 17, 2019

Conversation

TatriX
Copy link
Member

@TatriX TatriX commented Dec 11, 2019

Hi!
I was readining a new version of the guide and noticed several possible minor issues.
I'll put proposed changes here.

@TatriX TatriX changed the title Update structure.md Minor fixes Dec 11, 2019
Text says that `update` function returns `Update` struct. But in the code example it actually doesn't. I'm not yet sure how to fix it.
@TatriX
Copy link
Member Author

TatriX commented Dec 11, 2019

There are links to API docs in the text but

  1. They point to outdated versions
  2. Docs for 0.5 are missing.

@MartinKavik
Copy link
Member

@MartinKavik
Copy link
Member

@TatriX

  • Could you change

    to on: [push, pull_request], please? It's OT, but it would be nice to have linters enabled for this and next PRs.
  • Let us know, once this PR is ready for code review.

@TatriX
Copy link
Member Author

TatriX commented Dec 11, 2019

Sure. Note that you can push to my branch with your own changes.

@David-OConnor
Copy link
Member

David-OConnor commented Dec 12, 2019

Say when ready, and we'll merge this. These fixes are great.

@TatriX
Copy link
Member Author

TatriX commented Dec 13, 2019

Sure, I'm slowly reading through the docs, so when I'll finish I'll leave a comment.
But you may squash and merge if you want.

@TatriX
Copy link
Member Author

TatriX commented Dec 16, 2019

Ok, I think this PR can be squashed and merged.
It seems that deployed site is a bit different from master so I'll probably go through the text again in the future.

@MartinKavik
Copy link
Member

It seems that deployed site is a bit different from master

It sounds like a bug, master should be deployed automatically. What's different?

@TatriX
Copy link
Member Author

TatriX commented Dec 16, 2019

Hm, I can't see the difference anymore. Either some caching problem or just my misunderstanding.

fetch.md has an app.update(Msg::FetchData); in the first code example and it seems it's staled. Could you please take a look?

@MartinKavik
Copy link
Member

fetch.md has an app.update(Msg::FetchData); in the first code example and it seems it's staled. Could you please take a look?

I don't see any differences. Maybe it's really a problem with cache.
Last deploys

  • Dec 11 at 5.20 PM
  • Dec 10 at 12.12 AM and 12.10 AM
  • Dec 4, at 11.54 PM

If it works for you now, please create a new issue like "Implement cache busting" in quickstart-webpack repo.

@TatriX
Copy link
Member Author

TatriX commented Dec 16, 2019

Sorry, I wasn't clear enough. What I mean when I asked to take a look is that code example itself if staled.
It uses non-existing Msg::FetchData. It seems that it should be removed from the code and from the explanation.

@MartinKavik
Copy link
Member

@TatriX Yeah, thanks for the explanation.
I think there is/will be more similar cases because I don't know how to check them during compilation - any ideas?

@TatriX
Copy link
Member Author

TatriX commented Dec 16, 2019

Well, I would do that with help of org-mode + org-babel instead of markdown (or any other literate programming tool). This way all code can be compiled and thus checked. Though I'm not sure this is acceptable for our use case, because it will make contribution process harder.

@MartinKavik
Copy link
Member

Some ideas here: #11. We should continue discussion in that issue.

@David-OConnor David-OConnor merged commit 24d724b into seed-rs:master Dec 17, 2019
@TatriX TatriX deleted the patch-1 branch December 17, 2019 09:28
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