Skip to content

feat(system): add cron#897

Merged
Shinigami92 merged 17 commits into
faker-js:mainfrom
nhammond101:main
Aug 29, 2022
Merged

feat(system): add cron#897
Shinigami92 merged 17 commits into
faker-js:mainfrom
nhammond101:main

Conversation

@nhammond101

Copy link
Copy Markdown
Contributor

Adds new functionality to generate cron expressions.

New Function:

  • faker.system.cron()

Usage:

faker.system.cron():

// generates a random cron expression
console.log(faker.system.cron()): // '45 23 * * 6'

supports an optional options object, which currently has a year component

// generates a random cron expression with a year
console.log(faker.system.cron()): // '45 23 * * 6 2067'

Unit tests for the functions added.

@nhammond101 nhammond101 requested a review from a team as a code owner May 1, 2022 10:47
@codecov

codecov Bot commented May 1, 2022

Copy link
Copy Markdown

Codecov Report

Merging #897 (e73eca9) into main (7f8b871) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #897      +/-   ##
==========================================
- Coverage   99.63%   99.62%   -0.01%     
==========================================
  Files        2160     2160              
  Lines      240541   240614      +73     
  Branches     1014     1015       +1     
==========================================
+ Hits       239652   239710      +58     
- Misses        868      883      +15     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/system/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 82.53% <0.00%> (-3.97%) ⬇️
@Shinigami92 Shinigami92 added the c: feature Request for new feature label May 1, 2022
@Shinigami92 Shinigami92 added this to the v6.3 - Next Minor milestone May 1, 2022
@Shinigami92

Copy link
Copy Markdown
Member

Targeting it for v6.3, but I don't think it will make it so fast
We plan to work on v7 in next "sprint"

@ST-DDT ST-DDT added good first issue Good for newcomers p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels May 1, 2022

@Shinigami92 Shinigami92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we also try to generate some more advanced cron strings like 23 0-20/2 * * *?

Comment thread src/system.ts Outdated
@ST-DDT ST-DDT removed the good first issue Good for newcomers label May 1, 2022
@ST-DDT

ST-DDT commented May 1, 2022

Copy link
Copy Markdown
Member

Should we add some range for the cron: E.g. yearly, daily, ... (nothing precise, just some generaly direction)

Comment thread src/system.ts Outdated
Comment thread src/system.ts Outdated
Comment thread src/system.ts Outdated
Comment thread src/system.ts Outdated
@nhammond101

Copy link
Copy Markdown
Contributor Author

Should we also try to generate some more advanced cron strings like 23 0-20/2 * * *?

i'd like to, I wanted to get the simplest expressions generated first and potentially gauge demand

@ejcheng ejcheng added the needs rebase There is a merge conflict label May 6, 2022
@Shinigami92 Shinigami92 removed the needs rebase There is a merge conflict label May 23, 2022
@Shinigami92 Shinigami92 changed the title feat: Add cron() function feat: add system.cron May 23, 2022
@Shinigami92 Shinigami92 requested review from ST-DDT and Shinigami92 May 23, 2022 19:32
Comment thread src/modules/system/index.ts Outdated

@Shinigami92 Shinigami92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR needs to update their test but we found out that to many other stuff not related to this PR within the test file is messed up
So it will not be merged in v7.0.0

@xDivisionByZerox xDivisionByZerox added m: system Something is referring to the system module needs rebase There is a merge conflict labels Jul 28, 2022
@Shinigami92 Shinigami92 changed the title feat: add system.cron feat(system): add cron Aug 12, 2022
@ST-DDT

ST-DDT commented Aug 16, 2022

Copy link
Copy Markdown
Member

@nhammond101 Could you please update this PR?

@ST-DDT ST-DDT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think these are the last things that needs to be changed.
Otherwise it looks good to me.

Comment thread src/modules/system/index.ts Outdated
@Shinigami92 Shinigami92 self-requested a review August 26, 2022 13:57

@Shinigami92 Shinigami92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would have solved it differently and maybe more readably, but my case/request are tackled and solved 👍

@ST-DDT ST-DDT requested review from a team August 26, 2022 14:03
@nhammond101

Copy link
Copy Markdown
Contributor Author

@Shinigami92 if you get a few minutes, would you post your approach?

@Shinigami92

Copy link
Copy Markdown
Member

if you get a few minutes, would you post your approach?

See here: #897 (comment)

Comment thread src/modules/system/index.ts
@Shinigami92 Shinigami92 enabled auto-merge (squash) August 29, 2022 11:28
@Shinigami92 Shinigami92 merged commit 8fecd58 into faker-js:main Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: feature Request for new feature m: system Something is referring to the system module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

5 participants