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

Improve documentation #256

Merged
merged 2 commits into from
Oct 9, 2021
Merged

Improve documentation #256

merged 2 commits into from
Oct 9, 2021

Conversation

pinksynth
Copy link
Contributor

Hello, thank you for this awesome library!

In the proposed changes, I moved the pathToRegexp function documentation to its own section so that it follows the structure of the other functions. I added a little clarification, and an example for using named parameters with match (it took me a while to understand how to use this lib for named capture groups).

Hope this is helpful!

Moved `pathToRegexp` function documentation to its own section, like the other functions. Added some clarification, and an example for using named parameters with `match`.
Copy link
Member

@jonchurch jonchurch left a comment

Choose a reason for hiding this comment

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

I think this looks solid. Can you revert the header changes, unless you have a reason they are included? I'm assuming it was an autofix markdown linter, and not intentional.

Readme.md Outdated
@@ -194,6 +198,21 @@ fn("/invalid"); //=> false
fn("/user/caf%C3%A9"); //=> { path: '/user/caf%C3%A9', index: 0, params: { id: 'café' } }
```

The `match` function can be used to custom match named parameters (as of Node 10.0.0). For example, this can be used to whitelist a small number of valid paths:
Copy link
Member

Choose a reason for hiding this comment

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

I personally don't know what you mean by "(as of Node 10.0.0)" here.
I ran the example code under Node v9.11.2 and got the same output. Can you elaborate for me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I must've been confusing that limitation with that of another function:
#225

Will make the change.

@jonchurch jonchurch added the docs label Jul 21, 2021
@pinksynth
Copy link
Contributor Author

pinksynth commented Jul 22, 2021

Can you revert the header changes, unless you have a reason they are included?

@jonchurch I was treating the "Parameters" section, "Named parameters" section, etc, as parts of the pathToRegexp documentation, so I increased their header level. But now I think those sections apply to other functions as well though, so I reverted the change. Latest version should be good.

Copy link
Member

@jonchurch jonchurch left a comment

Choose a reason for hiding this comment

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

LGTM. It might take some time before the maintainer merges this in, assuming they don't have requests of their own. But thank you for your time and efforts to improve the documentation!

@@ -194,6 +198,21 @@ fn("/invalid"); //=> false
fn("/user/caf%C3%A9"); //=> { path: '/user/caf%C3%A9', index: 0, params: { id: 'café' } }
```

The `match` function can be used to custom match named parameters. For example, this can be used to whitelist a small number of valid paths:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `match` function can be used to custom match named parameters. For example, this can be used to whitelist a small number of valid paths:
The `match` function can be used with custom match named parameters. For example, this can be used to whitelist a small number of valid paths:

Is the goal of this section just to demonstrate you can use all the same paths as the previous documentation sections? Would it be better just to say that explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is mostly just to demonstrate how the lib could be used to name some matched parameters. With the existing docs, it took me a while to understand that :tab(about|photos) would actually provide a tab param with the matched value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants