Skip to content

Conversation

@mathbunnyru
Copy link
Member

No description provided.

rev: v0.27.1
hooks:
- id: markdownlint
args: ['--fix']
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes some errors in an automatic way.

# MD013/line-length - Line length
MD013:
# Number of characters
line_length: 1000
Copy link
Member Author

Choose a reason for hiding this comment

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

This is better to be fixed in a separate PR (it will be too much noise here)

@consideRatio
Copy link
Member

This LGTM!

I have become very fond of an autoformatter for markdown called prettier, but I have no experience with markdown-cli to compare it with. Due to that, I suggest that deliberating on that isn't a blocker for merging this PR. Briefly though, we have used prettier across the jupyterhub organization for YAML, JSON, and Markdown autoformatting - here is an example from a .pre-commit-config.yml:

  # Autoformat: markdown, yaml
  - repo: https://github.com/pre-commit/mirrors-prettier
    rev: v2.2.1
    hooks:
      - id: prettier

@mathbunnyru
Copy link
Member Author

mathbunnyru commented May 2, 2021

This LGTM!

I have become very fond of an autoformatter for markdown called prettier, but I have no experience with markdown-cli to compare it with. Due to that, I suggest that deliberating on that isn't a blocker for merging this PR. Briefly though, we have used prettier across the jupyterhub organization for YAML, JSON, and Markdown autoformatting - here is an example from a .pre-commit-config.yml:

  # Autoformat: markdown, yaml
  - repo: https://github.com/pre-commit/mirrors-prettier
    rev: v2.2.1
    hooks:
      - id: prettier

Thanks, @consideRatio. Prettier looks nice. I will take a closer look after this PR gets merged.

@mathbunnyru mathbunnyru requested a review from romainx May 2, 2021 20:40
@mathbunnyru
Copy link
Member Author

@romainx could you please take a look here?

Also, I had one approval from @consideRatio, which makes me believe that this is ready to be merged :)

@mathbunnyru
Copy link
Member Author

The reason I'm asking - this blocks me from further docs improvement (from the technical point of view).

@romainx
Copy link
Collaborator

romainx commented May 6, 2021

@mathbunnyru there is a conflict now could you solve it?
Any advice on how to review all the changes to the docs?
I guess generate the doc locally and check?

@consideRatio
Copy link
Member

consideRatio commented May 6, 2021

Any advice on how to review all the changes to the docs?
I guess generate the doc locally and check?

RTD pr preview would be nice to activate, requires maintainer permissions on the associated RTD project though.

@parente @minrk @willingc
could you add @romainx and @mathbunnyru who is a maintainer / about to be (#1276) of this project to the RTD project?

https://readthedocs.org/projects/jupyter-docker-stacks/

@mathbunnyru
Copy link
Member Author

@mathbunnyru there is a conflict now could you solve it?

Fixed.

Any advice on how to review all the changes to the docs?
I guess generate the doc locally and check?

Yes, for now local generation (though we need to use old versions).
Or, just check that the syntax is correct, it should work fine :)

RTD pr preview would be great 👍

@consideRatio
Copy link
Member

@mathbunnyru and @romainx I don't remember how it works properly, but perhaps you can make sure you have a RTD account (or have signed in once using your github handle to RTD)?

@mathbunnyru
Copy link
Member Author

@mathbunnyru and @romainx I don't remember how it works properly, but perhaps you can make sure you have a RTD account (or have signed in once using your github handle to RTD)?

I've already created one and linked it with my GitHub account and the login is the same mathbunnyru.
So, waiting for someone to give me some rights :)

@romainx
Copy link
Collaborator

romainx commented May 6, 2021

@consideRatio same for me.

Copy link
Collaborator

@romainx romainx left a comment

Choose a reason for hiding this comment

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

Finally I reviewed the files manually 😅. Did not catch any error 🕵️ .

@romainx romainx merged commit 59c973d into jupyter:master May 6, 2021
@romainx
Copy link
Collaborator

romainx commented May 6, 2021

Thanks @mathbunnyru always improve!

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