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(book): include PRQL changelog in book #2348

Merged
merged 11 commits into from
Apr 2, 2023

Conversation

eitsupi
Copy link
Member

@eitsupi eitsupi commented Mar 30, 2023

Close #2344

Since prql code blocks in md files included in the Book will evaluated and replace with html, this PR introduce a new Option no-eval that modifies the Book preprocessor to not evaluate Changelog's prql code blocks.
Also add a pre-commit hook to avoid using bare prql code blocks on CHANGELOG.md.

CHANGELOG.md Outdated
@@ -233,17 +233,21 @@ This release has 74 commits from 12 contributors. Selected changes:
formats. _format-arg_ can be `format:csv` or `format:json`. _string-arg_ can
be a string in any format. (@aljazerzen & @snth, #1514)

```prql
```elm
Copy link
Member

Choose a reason for hiding this comment

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

Should these never be prql?

Maybe we add a grep lint in pre-commit if so; otherwise folks are going to forget?

Or are they OK / actually good in prql?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your review.
The problem here is that in the book the prql code blocks are evaluated by the preprocessor and displayed alongside the sql code blocks.

If we could disable the evaluation of prql code blocks throughout the particular file with ignore settings, special comments or yaml front matter, I think we could continue to use prql code blocks there.
Is that possible now?

Maybe we add a grep lint in pre-commit if so; otherwise folks are going to forget?

I did not know of a solution that would make that possible. This is a good method at this stage!

Copy link
Member

Choose a reason for hiding this comment

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

prql_no_test will skip over creating a comparison table: https://github.com/prql/prql/blob/13d0060dfe80ee3ab098b85207ef6fae9d367a55/web/book/src/lib.rs#L155

(Though do we sometimes want a comparison table?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the issue here is the gap between rendering on GitHub and rendering on Book.
That is, if prql code blocks are recognized on GitHub in the future (#1636), Changelog will need to use prql code blocks, but may not want to output sql on the Book.

Copy link
Member Author

Choose a reason for hiding this comment

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

Considering that the syntax has changed and past prql code block can no longer be compiled, Changelog still needs to disable prql code evaluation on a file-by-file basis.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I actually thought I'd make a change to use a list, quick PR at https://github.com/PRQL/prql/pull/2350/files

(doesn't affect this PR though)

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, wonderfully fast work, thanks!
So can we merge this PR now?

Copy link
Member

Choose a reason for hiding this comment

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

Yes!

Copy link
Member

Choose a reason for hiding this comment

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

What should the instructions for including change blocks here be? "Always use elm"? "Only use prql if you want a comparison table"?

Copy link
Member Author

Choose a reason for hiding this comment

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

What should the instructions for including change blocks here be? "Always use elm"? "Only use prql if you want a comparison table"?

Good point.

It seems we need to stay with elm for now.
For now, it seems that when the prql code block is evaluated, a header PRQL is added and the display is corrupted.

image

prql/CHANGELOG.md

Lines 315 to 327 in fe1b7d4

- _Experimental:_ The
[`case`](https://prql-lang.org/book/language-features/case.html) function sets
a variable to a value based on one of several expressions (@aljazerzen,
#1278).
```prql
derive var = case [
score <= 10 -> "low",
score <= 30 -> "medium",
score <= 70 -> "high",
true -> "very high",
]
```

@max-sixty
Copy link
Member

max-sixty commented Mar 30, 2023

One thing we want to avoid here is:

  • Someone adds an entry to the Changelog with a prql block; the code doesn't compile
  • This is only picked up when building the book, not in the book tests, since the book tests only look at the .md files in the book. We run the book tests every commit, but we only build the book in build-web
  • Then build-web fails next time someone makes a small change to the web because of that issue, and we have to unravel what caused it, and we're reverting big PRs because of a small changelog problem.

We could run build-web on changes to the changelog; I think that would solve it

max-sixty added a commit to max-sixty/prql that referenced this pull request Mar 30, 2023
max-sixty added a commit that referenced this pull request Mar 30, 2023
* refactor: Use tags for the language code blocks

Idea from #2348

* change to spaces
@eitsupi
Copy link
Member Author

eitsupi commented Mar 31, 2023

I added no-eval tag to skip evaluate the code block in 9e89bbb.
This should work the same way as the elm on the book, so it can be used as a perfect replacement for the elm code block.
(Of course, we could continue to use elm.)

@max-sixty
Copy link
Member

Perfect!

Should we also add a https://pre-commit.com/#pygrep so no one puts a prql$ tag in the Changelog?

@eitsupi
Copy link
Member Author

eitsupi commented Apr 1, 2023

Should we also add a pre-commit.com/#pygrep so no one puts a prql$ tag in the Changelog?

I added that in 8347d17 and it seems working fine de5c5ff

@eitsupi
Copy link
Member Author

eitsupi commented Apr 1, 2023

It would be great if we could automatically link to issue numbers and GitHub user accounts like pkgdown does (https://pkgdown.r-lib.org/news/index.html), but my skills are not currently up to the task of implementing this...
https://github.com/r-lib/pkgdown/blob/c354aa7e5ea1f9936692494c28c89e5bdd31fc68/R/repo.R#L39-L46

@eitsupi eitsupi requested a review from max-sixty April 2, 2023 00:32
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@max-sixty
Copy link
Member

It would be great if we could automatically link to issue numbers and GitHub user accounts like pkgdown does (https://pkgdown.r-lib.org/news/index.html), but my skills are not currently up to the task of implementing this... https://github.com/r-lib/pkgdown/blob/c354aa7e5ea1f9936692494c28c89e5bdd31fc68/R/repo.R#L39-L46

Yes totally. A challenge for the future (...generations...)!

@max-sixty max-sixty enabled auto-merge (squash) April 2, 2023 00:36
@max-sixty
Copy link
Member

I hit the button to get this in for 0.7.0, hope that's OK!

@max-sixty max-sixty merged commit 2125923 into PRQL:main Apr 2, 2023
@max-sixty
Copy link
Member

In! Thanks @eitsupi !

@eitsupi
Copy link
Member Author

eitsupi commented Apr 2, 2023

Thanks!

@eitsupi eitsupi deleted the changelog-in-book branch April 2, 2023 03:21
max-sixty added a commit to max-sixty/prql that referenced this pull request Apr 2, 2023
* refactor: Use tags for the language code blocks

Idea from PRQL#2348

* change to spaces
max-sixty added a commit to max-sixty/prql that referenced this pull request Apr 2, 2023
Close PRQL#2344

Since `prql` code blocks in md files included in the Book will evaluated
and replace with html, this PR introduce a new Option `no-eval` that
modifies the Book preprocessor to not evaluate Changelog's `prql` code
blocks.
Also add a pre-commit hook to avoid using bare `prql` code blocks on
`CHANGELOG.md`.

---------

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

Upload the changelog to the web
2 participants