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

docs(manuals): extensive rewrite #11825

Merged
merged 3 commits into from Jan 14, 2020
Merged

docs(manuals): extensive rewrite #11825

merged 3 commits into from Jan 14, 2020

Conversation

papb
Copy link
Member

@papb papb commented Jan 14, 2020

docs(manuals): extensive rewrite

Finally! This is the PR I have been working on for a few months.

I have basically rewritten the whole manuals. This will solve several issues at once.

Summary

  • The docs folder was reorganized;

  • Almost every manual was rewritten;

  • Several new manuals were added;

  • A lot more examples were provided;

  • And I took care not to break any existing links!

    • Two of the manuals were renamed:

      • manual/dialects.html changed to manual/dialect-specific-things.html;
      • manual/instances.html changed to manual/model-instances.html.

      To make sure the old links for these two still work, I have set up a permanent redirect for the old URLs to the new ones. See docs/redirects and docs/redirects.json.

    • The following five manuals were split into several pages, with a lot more details:

      • associations.md;
      • data-types.md;
      • models-definition.md;
      • models-usage.md;
      • querying.md

    Again, to make sure every link that is around the internet is still valid and useful, each of these pages became a mini-page linking for the new guides. See for example docs/manual/moved/associations.md.

Consequences

This PR will not only make me a lot more comfortable with the Sequelize guides in general, but will also close several issues!

@papb papb requested a review from sushantdhiman Jan 14, 2020
@papb papb added status: awaiting maintainer type: docs labels Jan 14, 2020
@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #11825 into master will decrease coverage by 2.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11825      +/-   ##
==========================================
- Coverage   96.26%   94.18%   -2.08%     
==========================================
  Files          94       94              
  Lines        9191     9191              
==========================================
- Hits         8848     8657     -191     
- Misses        343      534     +191
Impacted Files Coverage Δ
lib/model.js 96.54% <ø> (ø) ⬆️
lib/sequelize.js 92.81% <ø> (-3.13%) ⬇️
lib/dialects/mysql/query.js 15.12% <0%> (-83.2%) ⬇️
lib/dialects/mysql/connection-manager.js 29.09% <0%> (-67.28%) ⬇️
lib/dialects/mysql/data-types.js 43.75% <0%> (-54.69%) ⬇️
lib/dialects/mysql/query-generator.js 94.8% <0%> (-3.04%) ⬇️
lib/dialects/abstract/query.js 91.66% <0%> (-0.33%) ⬇️
lib/dialects/abstract/query-generator.js 96.84% <0%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3110e3...23d1a42. Read the comment docs.

include: [
[
// Note the wrapping parentheses in the call below!
sequelize.literal(`(
Copy link
Contributor

@sushantdhiman sushantdhiman Jan 14, 2020

Choose a reason for hiding this comment

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

Hmm, include support this? Do we have any tests for this case?

Copy link
Member Author

@papb papb Jan 14, 2020

Choose a reason for hiding this comment

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

Good catch. We don't have any tests for this currently. However, it does work!! Should I just add one test for this? Or do you think this shouldn't be officially supported for some reason? I think it's great that it works :)

Copy link
Contributor

@sushantdhiman sushantdhiman Jan 14, 2020

Choose a reason for hiding this comment

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

Keep it

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Looks really good. Left some comments, I'll run this locally in my evening time for final review.

@sushantdhiman sushantdhiman added status: awaiting response status: in discussion and removed status: awaiting maintainer labels Jan 14, 2020
@sushantdhiman sushantdhiman merged commit 8fe367e into master Jan 14, 2020
3 of 4 checks passed
@sushantdhiman sushantdhiman deleted the manuals-rewrite branch Jan 14, 2020
@jly36963
Copy link

jly36963 commented Jan 14, 2020

I have basically rewritten the whole manuals. This will solve several issues at once.

@papb Thank you for doing this :D

@papb papb removed status: awaiting response status: in discussion labels Jan 14, 2020
@papb
Copy link
Member Author

papb commented Jan 14, 2020

For those wondering, the docs website may take at most 48 hours to be updated.

@sushantdhiman Can you manually trigger an update?

@papb
Copy link
Member Author

papb commented Jan 15, 2020

This also closed #11365, I forgot to put it in the list!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment