-
Notifications
You must be signed in to change notification settings - Fork 9
This PR changes the puzzle fetching logic to use the external ChessGubbins API and introduces difficulty level filtering based on user settings. #156
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
Conversation
… to user settings for filtering puzzle based on difficulty
hello @sandeepbhanja thanks for the PR, please do fix the build run in the settingSchema class |
Hi , I had forgotten to push a file , I have made the changes , the build run has passed. |
You also need to update contributing.md and Readme regarding the use of chessgubbins so it's better documented. |
Hi , I have updated the files |
looks great, I will test it locally and merge after, no worries, these PR helps alot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left few changes that need to be addressed, right now the puzzle search fails on edge cases and its not prod ready yet
@@ -93,8 +92,8 @@ public void registerSlashMultipleOptionCommand() { | |||
new OptionData(OptionType.STRING, "difficulty", "pick the engine difficulty", true) | |||
.addChoice("Easy", "5").addChoice("Medium", "10").addChoice("Hard", "15")); | |||
|
|||
buildSlashMultipleOption("setting", "set Chesslise settings like board theme & piece type", | |||
SettingSchema.getBoardThemeData(), SettingSchema.getPieceTypeData()); | |||
buildSlashMultipleOption("setting", "set Chesslise settings like board theme, piece type and difficulty level", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make it "puzzle difficulty" in general difficulty can mean alot of things in chess
@@ -44,4 +48,11 @@ public static OptionData getPieceTypeData() { | |||
.addChoice("pixel", "pixel"); | |||
} | |||
|
|||
public static OptionData getDifficultyLevelType(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we follow the same naming scheme it should be getPuzzleDifficultyData()
JSONParser parser = new JSONParser(); | ||
JSONObject jsonObject = (JSONObject) parser.parse(response.body()); | ||
int difficultyLevel = Integer.parseInt(jsonObject.get("rating").toString()); | ||
if(!isValidDifficulty(difficultyLevel, settingSchema.getDifficultyLevel())){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When tested locally, the code breaks if the user hasn't set any puzzle settings, which is null. This is likely the case for most users, as they are not aware that such a new setting exists, resulting in an infinite while loop. Can you make sure you check for nulls in the settingSchema
private static final String puzzleUrl = "https://api.chessgubbins.com/puzzles/random?themes="; | ||
|
||
private static final HttpClient client = HttpClient.newHttpClient(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function fails for opening theme for some reason, can you address why is that if the API doesn't support the theme it should revert to local search
private static final HttpClient client = HttpClient.newHttpClient(); | ||
|
||
|
||
private static List<List<String>> searchPuzzles(String searchColumn, String searchValue, int maxPuzzles, String userId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename this to searchPuzzlesGubbinsApi or something similar it is confusing if its searching locally or via API
@@ -8,11 +8,13 @@ public class SettingSchema { | |||
private final String userid; | |||
private final String boardTheme; | |||
private final String pieceType; | |||
private final String difficultyLevel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle null case please
@@ -13,9 +13,9 @@ public class ThemePuzzle extends PuzzleView implements Puzzle { | |||
private final String theme; | |||
private final LichessDBPuzzle currentPuzzle; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we let users know they can change the puzzle difficulty via settings in the theme card description? People might complain why they are only seeing easy puzzles
} else { | ||
return new SettingSchema("blue", "kosal", userid); | ||
return new SettingSchema("blue", "kosal", userid,"Easy"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make it set to medium, generally people don't like easy puzzles as the chesslise users want to improve and try harder puzzles
README.md
Outdated
@@ -32,6 +32,7 @@ Chesslise integrates with **multiple powerful chess APIs** to provide a high-qua | |||
- ♟️ [JDA 5 (Discord API)](https://github.com/DV8FromTheWorld/JDA) | |||
- ♟️ [Stockfish API](https://stockfish.online/) | |||
- ♟️ [ChessDB CN (Opening Book)](https://chessdb.cn/cloudbookc_info_en.html) | |||
- ♟️ [Chessgubbins Puzzle API](https://chessgubbins.com) – Provides random puzzles based on themes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need the description, follow how its done with other api links
CONTRIBUTING.md
Outdated
## Puzzle API Integration: Chessgubbins | ||
Chesslise now utilizes the [Chessgubbins Puzzle API](https://chessgubbins.com) to fetch random puzzles based on the LichessDB theme through the `LichessPuzzleSearch` class. This integration allows for more varied and updated puzzles. | ||
|
||
If the Chessgubbins API is temporarily unavailable, Chesslise falls back to using a local CSV database (`lichess_db_puzzle.csv`). When contributing to the project: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should mention devs should get the lichess_db_puzzle.csv from Lichess's puzzle db site and should download locally and test it,
I also suggest to add some test files for fetching the API and making sure it works, thats optional though |
Hi , I have fixed the issues and raised a PR |
No description provided.