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

fix(url_for): some improvement for url_for #307

Merged
merged 12 commits into from
Jan 12, 2024

Conversation

dimaslanjaka
Copy link
Contributor

@dimaslanjaka dimaslanjaka commented May 13, 2023

chore: some improvement for url_for

  1. make options as optional with default value as empty object
  2. add JSDoc
    • add sample usage inside plugin/scripts
  3. assign options fallback to an empty object
    • sometimes i saw syntax using url_for the options given as null, prevent useless error give options fallback to an empty object

Property 'fromEntries' does not exist on type 'ObjectConstructor'

Property 'matchAll' does not exist on type 'string'
- sometimes `postinstall` not triggered when using `yarn` v3
chore(url_for): make `options` as optional

- keep `empty object` for default options
- sometimes i saw someone plugin fill `options` null, to prevent useless errors we must give options fallback to `empty object`
@coveralls
Copy link

coveralls commented May 13, 2023

Coverage Status

coverage: 96.793% (+0.05%) from 96.74%
when pulling b045658 on dimaslanjaka:fix_url_for
into 460621e on hexojs:master.

@dimaslanjaka
Copy link
Contributor Author

@SukkaW review pls

package.json Outdated Show resolved Hide resolved
Copy link
Member

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

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

Everything else LGTM!

@dimaslanjaka dimaslanjaka changed the title chore(url_for): fix url helper chore(url_for): some improvement for url_for May 14, 2023
@dimaslanjaka dimaslanjaka requested a review from SukkaW May 14, 2023 07:24
trailing_index: true,
trailing_html: true
}, config.pretty_urls);
const prettyUrlsOptions = Object.assign(
Copy link
Member

Choose a reason for hiding this comment

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

You can keep the original code style here

Copy link
Member

Choose a reason for hiding this comment

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

Looks like he used prettier. If eslint doesn't fail, I guess it doesn't matter.

uiolee
uiolee previously approved these changes Dec 22, 2023
Copy link
Member

@uiolee uiolee left a comment

Choose a reason for hiding this comment

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

lgtm

@uiolee uiolee changed the title chore(url_for): some improvement for url_for fix(url_for): some improvement for url_for Dec 22, 2023
lib/url_for.ts Outdated Show resolved Hide resolved
[skip ci]

Co-authored-by: Sukka <[email protected]>
@uiolee uiolee merged commit b207833 into hexojs:master Jan 12, 2024
12 checks passed
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.

5 participants