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

Feat iaso connection nf #23

Merged
merged 15 commits into from
Jun 13, 2024
Merged

Feat iaso connection nf #23

merged 15 commits into from
Jun 13, 2024

Conversation

nazarfil
Copy link
Contributor

@nazarfil nazarfil commented Jun 6, 2024

Creates IASO API client as module to fetch

@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch 3 times, most recently from 2da8763 to 883d95a Compare June 7, 2024 07:33
@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch from ed9e772 to 3ae2c11 Compare June 10, 2024 05:59
@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch from 3ae2c11 to 7f5dd00 Compare June 10, 2024 06:00
@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch from 4af6343 to 765076c Compare June 10, 2024 06:16
@nazarfil nazarfil requested a review from qgerome June 10, 2024 06:27
@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch from b53e721 to bc414f8 Compare June 10, 2024 07:41
Copy link
Member

@qgerome qgerome left a comment

Choose a reason for hiding this comment

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

Could you document your classes & methods please?

openhexa/toolbox/iaso/api.py Outdated Show resolved Hide resolved
openhexa/toolbox/iaso/api_client.py Outdated Show resolved Hide resolved
openhexa/toolbox/iaso/iaso.py Outdated Show resolved Hide resolved
openhexa/toolbox/iaso/iaso.py Outdated Show resolved Hide resolved
openhexa/toolbox/iaso/iaso.py Outdated Show resolved Hide resolved
openhexa/toolbox/iaso/iaso.py Outdated Show resolved Hide resolved
@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch from d779f71 to 460a147 Compare June 10, 2024 13:15
@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch from 25fa62e to b6be97c Compare June 10, 2024 14:10
@nazarfil nazarfil requested a review from qgerome June 10, 2024 14:28
docs/iaso.md Outdated Show resolved Hide resolved
openhexa/toolbox/iaso/iaso.py Outdated Show resolved Hide resolved
@nazarfil nazarfil requested a review from qgerome June 11, 2024 08:08
Copy link
Member

@qgerome qgerome left a comment

Choose a reason for hiding this comment

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

Almost there !

raise ValueError("Values for org_units and projects cannot be empty lists")
params = kwargs
params.update({"page": page, "limit": limit, "org_units": org_units, "projects": projects})
response = self.api_client.request("POST", "/api/forms", data=params)
Copy link
Member

Choose a reason for hiding this comment

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

All the methods are available on the Session object.

Suggested change
response = self.api_client.request("POST", "/api/forms", data=params)
response = self.api_client.post("/api/forms", data=params)

params.update({"page": page, "limit": limit})
if as_dataframe:
params.update({"csv": "true"})
response = self.api_client.request("GET", "/api/instances", params=params)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response = self.api_client.request("GET", "/api/instances", params=params)
response = self.api_client.get("/api/instances", params=params)

response = self.api_client.request("GET", "/api/instances", params=params)
forms = pl.read_csv(io.StringIO(response.content.decode("utf-8")))[dataframe_columns]
return forms
response = self.api_client.request("GET", "/api/instances/", params=kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response = self.api_client.request("GET", "/api/instances/", params=kwargs)
response = self.api_client.get("/api/instances/", params=kwargs)

"""
params = kwargs
params.update({"page": page, "limit": limit})
response = self.api_client.request("GET", "/api/orgunits", params=kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response = self.api_client.request("GET", "/api/orgunits", params=kwargs)
response = self.api_client.get("/api/orgunits", params=params)

Copy link
Member

Choose a reason for hiding this comment

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

params=kwargs works since you just copy the reference into params and update the dictionary in-place (and thus the kwargs object) but I would prefer to update the kwargs directly OR using params here.


params = kwargs
params.update({"page": page, "limit": limit})
response = self.api_client.request("GET", "/api/projects", params=params)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response = self.api_client.request("GET", "/api/projects", params=params)
response = self.api_client.get("/api/projects", params=params)

@nazarfil nazarfil requested a review from qgerome June 13, 2024 07:21
@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch 2 times, most recently from eb9a855 to 33d405d Compare June 13, 2024 07:46
@nazarfil nazarfil force-pushed the feat-iaso_connection-nf branch from 33d405d to f9405ac Compare June 13, 2024 07:47
@qgerome qgerome merged commit a46e67e into main Jun 13, 2024
2 checks passed
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.

2 participants