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: fix html nits #13835

Merged
merged 7 commits into from
Mar 2, 2023
Merged

docs: fix html nits #13835

merged 7 commits into from
Mar 2, 2023

Conversation

317brian
Copy link
Contributor

@317brian 317brian commented Feb 22, 2023

Description

Really minor fixes like closing the <br /> tag in preparation for Docusaurus2, which doesn't like it when they're not closed.

This PR has:

  • been self-reviewed.

@jwitko
Copy link
Contributor

jwitko commented Feb 22, 2023

Hey @317brian I have #13747 failing on a static CI check for something that looks appropriate for this PR and I'm guessing it will fail for you as well?

> spellcheck
> mdspell --en-us --ignore-numbers --report '../docs/**/*.md' || (./script/notify-spellcheck-issues && false)

    ../docs/tutorials/tutorial-jupyter-index.md
       41 | er both try to use port `8888,` so start Jupyter on a different port 

>> 1 spelling error found in 227 files

Although to be fair I have no idea what its upset about? Seems to be complaining about this line

@317brian
Copy link
Contributor Author

Yeah, someone else was running into this earlier. It passes spellcheck locally, but for some reason the GHA doesn't like the position of the comma. It wants it outside of the `, which is technically correct but looks odd in the rendered output.

@abhishekrb19
Copy link
Contributor

abhishekrb19 commented Feb 23, 2023

@317brian @jwitko, yeah, I ran into the same spellcheck failure. spellcheck assumed there was a huge codeblock with an incorrect syntax because of an additional backtick that got accidentally introduced. It's now fixed in master; if you pull the top of master, that should have the fix #13838.

@@ -53,7 +53,7 @@ Edit the new file as follows:
4. Move the `format` definition from `parser.parseSpec` to an `inputFormat` definition in `ioConfig`.
5. Delete the `parser` definition.
6. Save the file.
<br>You can check the format of your new ingestion file against the [migrated example](#example-ingestion-spec-after-migration) below.
<br />You can check the format of your new ingestion file against the [migrated example](#example-ingestion-spec-after-migration) below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Break here isn't really needed. Reads better without it.

@@ -477,7 +477,7 @@ The `indexSpec` object can include the following properties:
|-----|-----------|-------|
|bitmap|Compression format for bitmap indexes. Should be a JSON object with `type` set to `roaring` or `concise`. For type `roaring`, the boolean property `compressRunOnSerialization` (defaults to true) controls whether or not run-length encoding will be used when it is determined to be more space-efficient.|`{"type": "roaring"}`|
|dimensionCompression|Compression format for dimension columns. Options are `lz4`, `lzf`, `zstd`, or `uncompressed`.|`lz4`|
|stringDictionaryEncoding|Encoding format for STRING value dictionaries used by STRING and COMPLEX&lt;json&gt; columns. <br>Example to enable front coding: `{"type":"frontCoded", "bucketSize": 4}`<br>`bucketSize` is the number of values to place in a bucket to perform delta encoding. Must be a power of 2, maximum is 128. Defaults to 4.<br>See [Front coding](#front-coding) for more information.|`{"type":"utf8"}`|
|stringDictionaryEncoding|Encoding format for STRING value dictionaries used by STRING and COMPLEX&lt;json&gt; columns. <br />Example to enable front coding: `{"type":"frontCoded", "bucketSize": 4}`<br />`bucketSize` is the number of values to place in a bucket to perform delta encoding. Must be a power of 2, maximum is 128. Defaults to 4.<br />See [Front coding](#front-coding) for more information.|`{"type":"utf8"}`|
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaks here do seem to be needed: at least the VS Code preview gets confused with newlines within a table.

Elsewhere in this table we use two breaks to create a clear separation between paragraphs. Should we do that here as well for consistency? The resulting text looks better.

<br>You can use this property to load segments with future start and end dates, where "future" is relative to the time when the Coordinator evaluates data against the rule. Defaults to `true`.
<br />- the segment interval overlaps the rule interval, or
<br />- the segment interval starts any time after the rule interval starts.
<br />You can use this property to load segments with future start and end dates, where "future" is relative to the time when the Coordinator evaluates data against the rule. Defaults to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

At the cost of extra per-line spacing, you can replace the <br /> above with two spaces and get the same effect in a more Markdown-ish way. Here and below.

@@ -45,7 +45,7 @@ Before you follow the steps in this tutorial, download Druid as described in the
```
2. If you're already running Kafka on the machine you're using for this tutorial, delete or rename the `kafka-logs` directory in `/tmp`.

> Druid and Kafka both rely on [Apache ZooKeeper](https://zookeeper.apache.org/) to coordinate and manage services. Because Druid is already running, Kafka attaches to the Druid ZooKeeper instance when it starts up.<br>
> Druid and Kafka both rely on [Apache ZooKeeper](https://zookeeper.apache.org/) to coordinate and manage services. Because Druid is already running, Kafka attaches to the Druid ZooKeeper instance when it starts up.<br />
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads better as:

> Druid and Kafka both rely on ...
>
> In a production environment ...

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the <br/>, but no harm either.

Copy link
Contributor

@paul-rogers paul-rogers left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -45,7 +45,7 @@ Before you follow the steps in this tutorial, download Druid as described in the
```
2. If you're already running Kafka on the machine you're using for this tutorial, delete or rename the `kafka-logs` directory in `/tmp`.

> Druid and Kafka both rely on [Apache ZooKeeper](https://zookeeper.apache.org/) to coordinate and manage services. Because Druid is already running, Kafka attaches to the Druid ZooKeeper instance when it starts up.<br>
> Druid and Kafka both rely on [Apache ZooKeeper](https://zookeeper.apache.org/) to coordinate and manage services. Because Druid is already running, Kafka attaches to the Druid ZooKeeper instance when it starts up.<br />
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the <br/>, but no harm either.

@317brian
Copy link
Contributor Author

The step in CI that's failing is the license check

Error: found 1 missing licenses. These licenses are reported, but missing in the registry
druid_module: avro-extensions, groupId: com.sun.activation, artifactId: jakarta.activation, version: 1.2.1, license: Eclipse Distribution License 1.0

It's unlikely to be caused by this PR since this PR only touches files in docs and not the avro-extension. The PR also doesn't add any new files that would need a license.

@vtlim vtlim merged commit b4b354b into apache:master Mar 2, 2023
@317brian 317brian deleted the html-nits branch March 2, 2023 19:19
317brian added a commit to 317brian/druid that referenced this pull request Mar 10, 2023
@clintropolis clintropolis added this to the 26.0 milestone Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants