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

Created test code for parsing JSON files into JSON objects and then into Game objects. #599

Merged
merged 21 commits into from
May 24, 2020

Conversation

noahklowden
Copy link
Contributor

I have written test code in a sandbox directory in src/wdl that can be used to test parsing JSON objects (Explained in further detail in #485.

This code currently faces compiler errors as I was unable to figure out how to integrate the necessary code from the game-state using the Makefile. I suspect that, because of the switch from Make to CMake, there is no longer a Makefile that can make the game-state files into executables for me to be able to use for the test.

However, apart from being unable to integrate the game-state functions, the code should be functional once the Makefile issue is resolved.

Closes #485.

Copy link
Contributor

@dtbukowski dtbukowski left a comment

Choose a reason for hiding this comment

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

Nice work. I tested the code before all of the game stuff got added and the code works. This code should work when the makefile error is fixed. However, I don't know enough about game state to fix the error of a missing game.h file.

src/wdl/sandbox/parser.c Show resolved Hide resolved
src/wdl/sandbox/parser.h Show resolved Hide resolved
Copy link
Contributor

@MaxineK36 MaxineK36 left a comment

Choose a reason for hiding this comment

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

This is a great start in terms of practicing with the json parsing library, but needs a couple modifications before it'll be useful in the sandbox. Also, I've suggested a stopgap solution to your makefile issue.

src/wdl/sandbox/Makefile Show resolved Hide resolved


/* DEBUG is 0 normally, 1 to print debugging statements */
#define DEBUG 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to change to 0 before this merges


/* Helper function: to print debugging statements only when debugging */
int my_print(char *string) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete these excess spaces (lines 14 and 18)

/* reads the input file */
fp = fopen(filename, "r");
assert(fp);
fread(buffer, 4096, 1, fp);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the file is empty?


game_t *parse_wdl(char* filename) {
FILE *fp;
char buffer[4096];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please declare a macro for 4096

src/wdl/sandbox/parser.c Show resolved Hide resolved
@dtbukowski
Copy link
Contributor

@MaxineK36 I had problems with implementing the workaround as you suggested. I got an error that says /usr/bin/ld: cannot find -l:ibgame-state.a when using -l:$libgame
Now I doubt that the makefile should be trying to find -l:ibgame-state.a, so I tried -lgame-state.a and that didn't work either. You sure that the library should be -l:$libgame?

Also I was pretty certain that having an empty file would result in reading 0 bytes. This stackoverflow should confirm that:
https://stackoverflow.com/questions/18255384/read-an-empty-file-with-fread

Also, I see that the this task is still part of sprint 2. Should this be changed? The ideal situation would (I'm guessing) to do something at the start of this sprint. But what action should be done now?

@MaxineK36 MaxineK36 self-requested a review May 23, 2020 02:49
@MaxineK36
Copy link
Contributor

@dtbukowski It's ok that this is still assigned to sprint2 since it's associated with a sprint2 issue.
@noahklowden Please make sure you're checking this regularly (or that you have email notifications set up) so that you can update as needed when changes are requested, and re-request our reviews once you've made the changes.

re: Makefile: It looks like there were just a few typos in the Makefile, but I just pushed a commit that should resolve this and allow "make" to function properly. Now that this is working, I'm going to go through and review this PR again.

Copy link
Contributor

@MaxineK36 MaxineK36 left a comment

Choose a reason for hiding this comment

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

Make is now working, and I've noticed a couple of other things that need fixing.


int add_rooms_to_game(json_object *rooms, game_t *g) {

//initializes json_objects for use in making room_t
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments referring to multiple lines of code should be block comments
(/* Some comment here */)

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed all of the code that should be block comments

src/wdl/sandbox/parser.c Show resolved Hide resolved
src/wdl/sandbox/parser.c Show resolved Hide resolved
@dtbukowski
Copy link
Contributor

I implemented all of the small fix-ups that Maxine did. An error message shows when there are no arguments. Also got rid of a warning by using the return result of fread to get number of bytes parsed, where if it equals zero then the file is empty.

… parsed from the returned game object. Also fixed fread file so that empty file text only displays when file is actually empty.
@noahklowden
Copy link
Contributor Author

I added actual code that will show the game object and rooms were properly parsed by the code. This should make the code complete for the purposes of being a demonstration of the ease of parsing JSON files into game objects (or any other type of objects we want to convert it to).

@MaxineK36 MaxineK36 self-requested a review May 24, 2020 03:45
Copy link
Contributor

@MaxineK36 MaxineK36 left a comment

Choose a reason for hiding this comment

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

Looking good! This should be helpful as you continue working on the parser for wdl++.

@MaxineK36 MaxineK36 merged commit 0f45867 into dev May 24, 2020
@MaxineK36
Copy link
Contributor

Issue Score: ✔️

Comments:
This is a very solid first PR, with an excellent summary. A few things to think about for next time:

  1. Make sure that, when you create a PR, the code is ready for review and passes all the tests. The Makefile problem on this PR should have been resolved (either on your own or with my help) before making a PR.
  2. When a pull request is merged, you should include a final comment summarizing what was done, and why the PR is being closed.

@noahklowden
Copy link
Contributor Author

This code has successfully been merged into the dev branch. It should now act as a basis for how we can implement parsing JSON files into code. With this, this pull request is officially closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write test WDL++ files and JSON parsers to test out parsing of JSON files for WDL purposes
3 participants