ChenKaiLiuG/life_generators
1
0
Fork 0
Code Issues 7 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Feat(fortune): ci add special event checker #50

Merged
tobiichi3227 merged 2 commits from feat/pr-event-check-ci into main 2025-02-06 09:25:23 +00:00
Conversation 18 Commits 2 Files Changed 5 +415 -331
tobiichi3227 commented 2025-01-26 14:09:14 +00:00 (Migrated from github.com)
Copy Link

In this PR, we added a CI check process.
When there is a push or pull request, it will verify whether the events are valid, such as checking for syntax errors, incorrect trigger dates, and more.

In this PR, we added a CI check process. When there is a push or pull request, it will verify whether the events are valid, such as checking for syntax errors, incorrect trigger dates, and more.
lifeadventurer commented 2025-01-26 14:43:50 +00:00 (Migrated from github.com)
Copy Link

Have you tested the workflow?

Have you tested the workflow?
tobiichi3227 commented 2025-01-26 15:49:33 +00:00 (Migrated from github.com)
Copy Link

The workflow ran successfully
https://github.com/LifeAdventurer/generators/actions/runs/12975393011/job/36186418331?pr=50

The workflow ran successfully https://github.com/LifeAdventurer/generators/actions/runs/12975393011/job/36186418331?pr=50
lifeadventurer commented 2025-01-26 16:26:51 +00:00 (Migrated from github.com)
Copy Link

I mean have you tested various wrong formats and will he be able to debug it?

I mean have you tested various wrong formats and will he be able to debug it?
tobiichi3227 commented 2025-01-27 01:53:37 +00:00 (Migrated from github.com)
Copy Link

Ohhhh

Ohhhh
tobiichi3227 commented 2025-02-02 09:01:30 +00:00 (Migrated from github.com)
Copy Link
{
    "event": "this should be place in static",
    "triggerDate": {
        "month": "6",
        "date": "21"
    },
    "status_index": "0",
    "goodFortunes": {
        "l_1_event": "123",
        "l_1_desc": "123",
        "l_2_event": "",
        "l_2_desc": ""
    },
    "badFortunes": {
        "r_1_event": "",
        "r_1_desc": "",
        "r_2_event": "",
        "r_2_desc": ""
    }
}
`triggerDate` missing `year`.
this event `this should be place in static` should be placed in `static_special.json`.

{
    "event": "this should be place in cyclical",
    "triggerDate": {
        "month": "5",
        "week": "2",
        "weekday": "1"
    },
    "status_index": "0",
    "goodFortunes": {
        "l_1_event": "123",
        "l_1_desc": "123",
        "l_2_event": "",
        "l_2_desc": ""
    },
    "badFortunes": {
        "r_1_event": "",
        "r_1_desc": "",
        "r_2_event": "",
        "r_2_desc": ""
    }
}
`triggerDate` missing `year`.
`triggerDate` missing `date`.
this event `this should be place in cyclical` should be placed in `cyclical_special.json`.

{
    "event": "incorrect year, month",
    "triggerDate": {
        "year": "-1",
        "month": "123",
        "date": "2"
    },
    "status_index": "0",
    "goodFortunes": {
        "l_1_event": "123",
        "l_1_desc": "123",
        "l_2_event": "",
        "l_2_desc": ""
    },
    "badFortunes": {
        "r_1_event": "",
        "r_1_desc": "",
        "r_2_event": "",
        "r_2_desc": ""
    }
}
`triggerDate.year` should be between 1 and 9999
`triggerDate.month` should be between 1 and 12

{
    "event": "incorrect date",
    "triggerDate": {
        "year": "2021",
        "month": "2",
        "date": "29"
    },
    "status_index": "0",
    "goodFortunes": {
        "l_1_event": "123",
        "l_1_desc": "123",
        "l_2_event": "",
        "l_2_desc": ""
    },
    "badFortunes": {
        "r_1_event": "",
        "r_1_desc": "",
        "r_2_event": "",
        "r_2_desc": ""
    }
}
`triggerDate.date` should be between 1 and 28

{
    "event": "Mukden incident",
    "triggerDate": {
        "year": "1931",
        "month": "9",
        "date": "18"
    },
    "status_index": "0",
    "goodFortunes": {
        "l_1_event": "123",
        "l_1_desc": "123",
        "l_2_event": "",
        "l_2_desc": ""
    },
    "badFortunes": {
        "r_1_event": "",
        "r_1_desc": "",
        "r_2_event": "",
        "r_2_desc": ""
    }
}
The date `1931/9/18` of `Mukden incident` is repeated.

This is the current error message.
The checker will list all errors as much as possible.

```json { "event": "this should be place in static", "triggerDate": { "month": "6", "date": "21" }, "status_index": "0", "goodFortunes": { "l_1_event": "123", "l_1_desc": "123", "l_2_event": "", "l_2_desc": "" }, "badFortunes": { "r_1_event": "", "r_1_desc": "", "r_2_event": "", "r_2_desc": "" } } `triggerDate` missing `year`. this event `this should be place in static` should be placed in `static_special.json`. { "event": "this should be place in cyclical", "triggerDate": { "month": "5", "week": "2", "weekday": "1" }, "status_index": "0", "goodFortunes": { "l_1_event": "123", "l_1_desc": "123", "l_2_event": "", "l_2_desc": "" }, "badFortunes": { "r_1_event": "", "r_1_desc": "", "r_2_event": "", "r_2_desc": "" } } `triggerDate` missing `year`. `triggerDate` missing `date`. this event `this should be place in cyclical` should be placed in `cyclical_special.json`. { "event": "incorrect year, month", "triggerDate": { "year": "-1", "month": "123", "date": "2" }, "status_index": "0", "goodFortunes": { "l_1_event": "123", "l_1_desc": "123", "l_2_event": "", "l_2_desc": "" }, "badFortunes": { "r_1_event": "", "r_1_desc": "", "r_2_event": "", "r_2_desc": "" } } `triggerDate.year` should be between 1 and 9999 `triggerDate.month` should be between 1 and 12 { "event": "incorrect date", "triggerDate": { "year": "2021", "month": "2", "date": "29" }, "status_index": "0", "goodFortunes": { "l_1_event": "123", "l_1_desc": "123", "l_2_event": "", "l_2_desc": "" }, "badFortunes": { "r_1_event": "", "r_1_desc": "", "r_2_event": "", "r_2_desc": "" } } `triggerDate.date` should be between 1 and 28 { "event": "Mukden incident", "triggerDate": { "year": "1931", "month": "9", "date": "18" }, "status_index": "0", "goodFortunes": { "l_1_event": "123", "l_1_desc": "123", "l_2_event": "", "l_2_desc": "" }, "badFortunes": { "r_1_event": "", "r_1_desc": "", "r_2_event": "", "r_2_desc": "" } } The date `1931/9/18` of `Mukden incident` is repeated. ``` This is the current error message. The checker will list all errors as much as possible.
tobiichi3227 commented 2025-02-02 09:03:52 +00:00 (Migrated from github.com)
Copy Link

@LifeAdventurer
Should I write a new CI for testing the event checker?

@LifeAdventurer Should I write a new CI for testing the event checker?
lifeadventurer commented 2025-02-02 14:46:04 +00:00 (Migrated from github.com)
Copy Link

You only need to ensure that the event checker works. There's no need for a CI to repeatedly verify a static event checker.

You only need to ensure that the event checker works. There's no need for a CI to repeatedly verify a static event checker.
tobiichi3227 commented 2025-02-02 14:59:16 +00:00 (Migrated from github.com)
Copy Link

OK.
test_custom.json
test_cyclical.json
test_static.json
test_structure.json
There are some test files.

OK. [test_custom.json](https://github.com/user-attachments/files/18632730/test_custom.json) [test_cyclical.json](https://github.com/user-attachments/files/18632732/test_cyclical.json) [test_static.json](https://github.com/user-attachments/files/18632733/test_static.json) [test_structure.json](https://github.com/user-attachments/files/18632734/test_structure.json) There are some test files.
tobiichi3227 commented 2025-02-03 12:21:57 +00:00 (Migrated from github.com)
Copy Link

Do you want to merge this pr?

Do you want to merge this pr?
lifeadventurer commented 2025-02-03 14:00:13 +00:00 (Migrated from github.com)
Copy Link

I once left a message here asking you to put all scripts in the scripts/ folder but it seems to be gone. Anyway, I would like to ask you to make this change.

I once left a message here asking you to put all scripts in the `scripts/` folder but it seems to be gone. Anyway, I would like to ask you to make this change.
tobiichi3227 commented 2025-02-03 14:30:00 +00:00 (Migrated from github.com)
Copy Link

OK.
No problem.
I'll do it right now.

OK. No problem. I'll do it right now.
tobiichi3227 commented 2025-02-04 09:14:30 +00:00 (Migrated from github.com)
Copy Link

Is this okay now?

Is this okay now?
lifeadventurer commented 2025-02-05 10:56:10 +00:00 (Migrated from github.com)
Copy Link

I originally just wanted to ask you to move the CI script there. And after all, the two files originally in dev/ are outdated. But you have already moved them there. So now we have to consider whether to deal with the two files originally in dev/ in the next PR (then you need to reverse) or just put them in scripts or even delete them directly.

I originally just wanted to ask you to move the CI script there. And after all, the two files originally in `dev/` are outdated. But you have already moved them there. So now we have to consider whether to deal with the two files originally in dev/ in the next PR (then you need to reverse) or just put them in scripts or even delete them directly.
tobiichi3227 commented 2025-02-05 12:14:48 +00:00 (Migrated from github.com)
Copy Link

I think we can delete it in this PR first and open another PR related to probability analysis to handle this.

I think we can delete it in this PR first and open another PR related to probability analysis to handle this.
lifeadventurer commented 2025-02-05 12:21:43 +00:00 (Migrated from github.com)
Copy Link

Please commit these resolve request changes (including deleting the files) after I review your pull request.

Please commit these resolve request changes (including deleting the files) after I review your pull request.
lifeadventurer (Migrated from github.com) requested changes 2025-02-05 12:37:31 +00:00
.github/workflows/check-events.yml
@@ -0,0 +1,35 @@
name: Check special events
lifeadventurer (Migrated from github.com) commented 2025-02-05 12:29:47 +00:00
Copy Link

Use checkout@v4

Use checkout@v4
.github/workflows/check-events.yml
@@ -0,0 +1,35 @@
name: Check special events
on:
lifeadventurer (Migrated from github.com) commented 2025-02-05 12:29:07 +00:00
Copy Link

Try to use paths, so we only need to run this workflow when files under fortune_generator/json/ are changed.

Try to use `paths`, so we only need to run this workflow when files under `fortune_generator/json/` are changed.
scripts/check-events.py
lifeadventurer (Migrated from github.com) commented 2025-02-05 12:37:11 +00:00
Copy Link

Overall, this is a well-structured addition that will help maintain data consistency. Here's my review:

Strong Points:

  • Well-organized code with proper typing, enums, and argument parsing
  • Comprehensive validation for different date types (Custom, Static, Cyclical)
  • Good error handling for file operations and JSON parsing
  • Thorough field checking with type validation
  • Clear error messaging system
  • Proper handling of leap years and date validation
  • Duplicate checking for event names and dates

Suggested Improvements:

  1. Code Structure:

    • Add docstrings to main functions for better documentation
    • Add type hints for function return values

  2. Error Handling:

    • Replace print statements with proper logging
    • Consider creating a custom Exception class for validation errors
    • Add input validation for the JSON file path

  3. Testing & Maintainability:

    • Would be great to add unit tests for the validation logic
    • Consider making the constants configurable via command line arguments

The code is good to merge after addressing the docstring and logging improvements. The other suggestions can be handled in future PRs.

Let me know if you'd like me to elaborate on any of these points.

Overall, this is a well-structured addition that will help maintain data consistency. Here's my review: Strong Points: - Well-organized code with proper typing, enums, and argument parsing - Comprehensive validation for different date types (Custom, Static, Cyclical) - Good error handling for file operations and JSON parsing - Thorough field checking with type validation - Clear error messaging system - Proper handling of leap years and date validation - Duplicate checking for event names and dates Suggested Improvements: 1. Code Structure: - Add docstrings to main functions for better documentation - Add type hints for function return values 2. Error Handling: - Replace print statements with proper logging - Consider creating a custom Exception class for validation errors - Add input validation for the JSON file path 3. Testing & Maintainability: - Would be great to add unit tests for the validation logic - Consider making the constants configurable via command line arguments The code is good to merge after addressing the docstring and logging improvements. The other suggestions can be handled in future PRs. Let me know if you'd like me to elaborate on any of these points.
scripts/main.js
lifeadventurer (Migrated from github.com) commented 2025-02-05 12:25:36 +00:00
Copy Link

Remove this file.

Remove this file.
scripts/plot_gen.py
lifeadventurer (Migrated from github.com) commented 2025-02-05 12:25:50 +00:00
Copy Link

Remove this file.

Remove this file.
lifeadventurer (Migrated from github.com) approved these changes 2025-02-06 08:48:43 +00:00
lifeadventurer (Migrated from github.com) left a comment
Copy Link

Squash your commits before I merge.

Squash your commits before I merge.
tobiichi3227 commented 2025-02-06 08:51:16 +00:00 (Migrated from github.com)
Copy Link

OK

OK
Sign in to join this conversation.
Reviewers
No Reviewers
lifeadventurer
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 stale
Pull Request is stale
 waiting for merge
waiting for review
waiting for update
wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
ChenKaiLiuG
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: ChenKaiLiuG/life_generators#50
Delete Branch "feat/pr-event-check-ci"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?