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

JSON/XML/CSV nil options #231

Merged
merged 12 commits into from Mar 28, 2023
Merged

Conversation

mgilbir
Copy link
Contributor

@mgilbir mgilbir commented Mar 27, 2023

This PR enables the generation of JSON and XML when the corresponding option objects are nil.

This is useful when one doesn't care about the structure of the generated JSON/XML as long as it is valid.

Some of the tests check for a well formed JSONOptions and fail when it is missing required parameters (eg. Fields or RowCount). While it may seem that a nil JSONOptions would be equally malformed, one can argue that when the options are nil it means that they are not specified instead of misspecified.

In that case, we can leverage the gofakeit mechanics to generate a random JSONOptions/XMLOptions, and proceed with the generation as usual.

Tests pass and code coverage is unchanged.

@brianvoe
Copy link
Owner

I think this looks great! nice job!

The only think i question is setting firstname or autoincrement on the Field. I almost think that one should be required.

Let me know your thoughts.

@mgilbir
Copy link
Contributor Author

mgilbir commented Mar 27, 2023

I chose for firstname and autoincrement to get strings and numbers in the field. Like this you can get some variety in the types of the generated data.

@brianvoe
Copy link
Owner

Let me test this one out a little bit. Cause i like the idea of not needing to pass in options to json and xml and having it automatically generate some for you. But I would like that generation to be a little bit more full of what feels like visually appealing data. you know what i mean?

@mgilbir
Copy link
Contributor Author

mgilbir commented Mar 27, 2023 via email

@brianvoe
Copy link
Owner

Ya from a programmers perspective I agree. as long as my code sees it as good then im good. But I do have a decent amount of people that use it for visuals. Things like front end. I am also building gofakeit.com and would like it if this feature had more visually appealing outputs as well.

@mgilbir
Copy link
Contributor Author

mgilbir commented Mar 27, 2023

Would an output like this be "visual" enough?

{
    "property_1": "Mary",
    "property_2": 42,
    ...
}

or do you want to go all the way to named properties like:

{
    "name": "Mary",
    "age": 42,
    ...
}

@brianvoe
Copy link
Owner

brianvoe commented Mar 27, 2023

ya that looks good. I would think randomly between 2 and 8 fields and maybe picking between the top 8 most used functions.

@mgilbir
Copy link
Contributor Author

mgilbir commented Mar 27, 2023

I tried something different.

It now picks a random number of existing parameterless FuncLookup and uses that to generate data.

An example looks like this:

[
  {
    "loglevel": "trace",
    "minecraftweapon": "shield",
    "minecraftbiome": "ocean",
    "streetprefix": "South",
    "minute": 40,
    "uint16": 47109,
    "cartransmissiontype": "Automatic",
    "language": "Portuguese",
    "dinner": "Ww shrimp scampi",
    "errorhttpserver": {}
  },
  {
    "loglevel": "error",
    "minecraftweapon": "sword",
    "minecraftbiome": "ice spike",
    "streetprefix": "Port",
    "minute": 24,
    "uint16": 7319,
    "cartransmissiontype": "Manual",
    "language": "Oromo",
    "dinner": "Blackened tuna bites with cajun mustard",
    "errorhttpserver": {}
  }
]

or

[
  {
    "dinner": "Cafe mocha latte",
    "hobby": "Shoemaking",
    "gamertag": "DullMacaw151",
    "adverb": "hardly",
    "uint32": 2581418833,
    "emojidescription": "black square button",
    "beerstyle": "Fruit Beer",
    "emojitag": "france",
    "pronounpersonal": "we",
    "bird": "ibis"
  },
  {
    "dinner": "Garlic prime rib",
    "hobby": "Bowling",
    "gamertag": "ConcerningPug",
    "adverb": "recently",
    "uint32": 2058102316,
    "emojidescription": "tangerine",
    "beerstyle": "Vegetable Beer",
    "emojitag": "lick",
    "pronounpersonal": "I",
    "bird": "duck"
  }
]

@mgilbir mgilbir changed the title JSON/XML nil options JSON/XML/CSV nil options Mar 27, 2023
@brianvoe
Copy link
Owner

I think this looks great! The only thing i would like to figure out is being able to get random lookup functions to run without needing to add internalFuncLookups. The lookup funcs are used in cmd, server and gofakeit.com. So adding to it would have it show up on the list of available functions to run.

@mgilbir
Copy link
Contributor Author

mgilbir commented Mar 28, 2023

I'm not sure I'm following. In particular I'm confused about the interest/value of having this particular function show up on the list of available functions to run from cmd, server and gofakeit.com.

The lookup function I added 'internal_ExampleFields' is returning a gofakeit.Field which I think limits the value of exposing it to end users automatically.

That is the reason why I added the internalFuncLookups so that it is possible to have a map with functions that are meant to be only used by gofakeit itself without exposing them to end users.

The reason to return a gofakeit.Field is because that is the value that we are trying to fake in the struct tag of JSONOptions/XMLOptions/CSVOptions:
49efc36#diff-e848e95f31309994dbd646cda14ba74ae0239fba72b388bb9cc0928057c50980R14

To sum up my understanding, and I may be missing something obvious, I see three ways of faking that value:

  • Expose a custom FuncLookup returning a gofakeit.Field to end users (IMO confusing unless they have a use for gofakeit.Field)
  • Have a custom FuncLookup returning a gofakeit.Field not exposed to end users (current implementation)
  • Use the mechanism in the other pull request (Custom fakeable types #230) to implement the gofakeit.Fakeable interface for gofakeit.Field (solves it without needing to introduce the internalFuncLookup)

What am I missing? 🤔

I hope I'm not being too obnoxious. I really appreciate you engaging and the project you've built! 😄

@brianvoe
Copy link
Owner

I think your right. I read the code too quickly. Its not adding to the lookup map its just able to retrieve by the get lookup func.

I think you understand my perspective pretty well so ill get this merged in.

Before I do can you update the README.md as well. people use that as reference and I think by adding a little comment near the json,xml and csv sections saying something like "you can pass nil and it will auto generate for you" would help finish it off.

I will wait to do a release once we hash out your other pr

@brianvoe brianvoe merged commit af9e266 into brianvoe:develop Mar 28, 2023
12 checks passed
@brianvoe
Copy link
Owner

Merged! Thanks for your contributions. I think people will enjoy the addition.

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

Successfully merging this pull request may close these issues.

None yet

2 participants