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

This is Michela's Pull Request - add documentation to IPUMS.jl #34

Open
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

00krishna
Copy link
Collaborator

This is Michela's original PR. But since her branch is located on her private repo, I did not have permissions to make changes there. Hence I had to create a new PR based upon her work. These are just some additional clean up and edits based upon @TheCedarPrince 's original comments.

MichelaRocchetti and others added 30 commits June 7, 2024 21:00
- added code example
- adjusted variable descriptions
- added code example
- add variable descriptions
- added references
- add Examples
-add description
-add References
-add Returns
-add description
-add examples
- add return
add description
add return
add ref
add examples
add description
add example
add reference
add return
add description
add reference
add examples
add arguments
add refernce
add description
add arguments add examples
add ref 
add description
add arguments
add examples
add description add reference add examples add return
add descruption add reference add examples add return add description
add returns 
add examples
add ref
add description
add description,examples,ref,return. To be checked
added code example to DataExtract.
added reference to ChatGPT.
- change definition
-suppress output
-keyword argument
examples in argument
fixed description and return 
missing example
definition and return changed
missing example and argument definition
ok description and return
missing example
ok description,examples,return ad arguments
MichelaRocchetti and others added 25 commits July 3, 2024 15:51
description and return changed
…d TimeSeriesTableFull files."

This reverts commit b0f2372.
…e, DatasetFullGeogLevelsInner, TimeSeriesTableSimple.
…tFullBreakdownsBreakdownValuesInner, and DatasetSimple.
…iesTableFull, and TimeSeriesTableFulltimeSeriesInner.
…oadLinks, DataExtractPost, DataExtractPostResponse, and Error.
@00krishna 00krishna marked this pull request as ready for review July 20, 2024 23:03
@00krishna 00krishna requested a review from TheCedarPrince July 21, 2024 21:08
Copy link
Member

@TheCedarPrince TheCedarPrince left a comment

Choose a reason for hiding this comment

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

Hey @MichelaRocchetti and @00krishna ,

This looks great! I only have one question: what are all the (Optional) statements for by so many arguments? My understanding is that if you don't provide any of these arguments with the (Optional) argument, then it would return a malformed statement back from the API endpoint.

Otherwise, if you can help me understand that, that is my only question.

Thanks!

~ tcp 🌳

@00krishna
Copy link
Collaborator Author

@TheCedarPrince Yeah, I think that all of the arguments are not needed every time for the api to work. I actually took the code examples from the nhgis.yml file in the repo. Many of those working examples seemed to ignore certain arguments, and apparently things work just fine.

Mechanically, the optional arguments are implemented as keyword arguments. So these keyword arguments have default values, like missing. So even if the user does not enter a value for each argument, the function still proceeds.

We are still in the process of testing all of these specific requests to make sure that they work. This is what @MichelaRocchetti was working on before she went on vacation. But we will continue with that to make sure that all requests check out.

Let me know if you have any additional questions.

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.

3 participants