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

feat(system): add cron #897

Merged
merged 17 commits into from
Aug 29, 2022
Merged

feat(system): add cron #897

merged 17 commits into from
Aug 29, 2022

Conversation

nhammond101
Copy link
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
Copy link

codecov bot commented May 1, 2022

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
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
Copy link
Member

@Shinigami92 Shinigami92 left a comment

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 * * *?

src/system.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT removed the good first issue Good for newcomers label May 1, 2022
@ST-DDT
Copy link
Member

ST-DDT commented May 1, 2022

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

src/system.ts Outdated Show resolved Hide resolved
src/system.ts Outdated Show resolved Hide resolved
src/system.ts Outdated Show resolved Hide resolved
src/system.ts Outdated Show resolved Hide resolved
@nhammond101
Copy link
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

@import-brain import-brain 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
src/modules/system/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@Shinigami92 Shinigami92 left a comment

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
Copy link
Member

ST-DDT commented Aug 16, 2022

@nhammond101 Could you please update this PR?

Copy link
Member

@ST-DDT ST-DDT 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 these are the last things that needs to be changed.
Otherwise it looks good to me.

src/modules/system/index.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 self-requested a review August 26, 2022 13:57
Copy link
Member

@Shinigami92 Shinigami92 left a comment

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
Contributor Author

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

@Shinigami92
Copy link
Member

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

See here: #897 (comment)

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants