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.

Sign up for GitHub

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

Jump to bottom

Fix bug add_poi_infos in journeys #63

Closed
wants to merge 1 commit into from
Closed

Fix bug add_poi_infos in journeys #63

wants to merge 1 commit into from

Conversation

SemiCroustii
Copy link

I add a bug when using

client.journeys.list_journeys(from_ = "stop_area:SNCF:REDACTED", to_ = "stop_area:SNCF:REDACTED"))

{
    "message": "parameter \"add_poi_infos[]\" invalid: The add_poi_infos[] argument must be in list ('bss_stands', 'car_park', '', 'none'), you gave []\nadd_poi_infos[] description: Show more information about the poi if it's available, for instance, show BSS/car park availability in the pois(BSS/car park) of response"
}

I changed add_poi_infos default value to "none"

jonperron
jonperron requested changes Jun 6, 2024
View reviewed changes
Copy link
Owner

@jonperron jonperron left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello @SemiCroustii

Thank you for your contribution.

I am requesting changes as your proposal is not the way the typing is used in the project. Indeed the value can be "none" but the type is Sequence[str], so it should be at least ["none"]. In addition, this value is optional in the call (which is indeed not described in https://doc.navitia.io/#journeys), can you use the Optional type to reflect this ?

Also, please add a test using the payload which you would have received as a response to your API call. You can use the RawClient if necessary, and clean the payload from useless data.

navitia_client/client/apis/journeys_apis.py
@@ -42,7 +42,7 @@ class JourneyApiClient(ApiBaseClient):
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

@jonperron jonperron Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Dito

navitia_client/client/apis/journeys_apis.py
@@ -79,7 +79,7 @@ class JourneyApiClient(ApiBaseClient):
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

Choose a reason for hiding this comment

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

Dito

navitia_client/client/apis/journeys_apis.py
@@ -116,7 +116,7 @@ class JourneyApiClient(ApiBaseClient):
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

Choose a reason for hiding this comment

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

Dito

navitia_client/client/apis/journeys_apis.py
@@ -173,7 +173,7 @@ def list_journeys(
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

Choose a reason for hiding this comment

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

Dito

navitia_client/client/apis/journeys_apis.py
@@ -333,7 +333,7 @@ def list_journeys_with_region_id(
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

Choose a reason for hiding this comment

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

Dito

navitia_client/client/apis/journeys_apis.py
@@ -495,7 +495,7 @@ def list_journeys_with_resource_path(
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

Choose a reason for hiding this comment

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

Dito

docs/api_support/journeys.md
@@ -37,7 +37,7 @@ Methods
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

@jonperron jonperron Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please use Optional if value is nullable (see https://docs.python.org/3/library/typing.html#typing.Optional).

Also well catched that "none" is a possibile value, as it is not listed in the api catalog. Could you set the value as ["none"] to match wit the Sequence typing ?

docs/api_support/journeys.md
@@ -74,7 +74,7 @@ Methods
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

Choose a reason for hiding this comment

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

Dito

docs/api_support/journeys.md
@@ -111,7 +111,7 @@ Methods
wheelchair: bool = False,
direct_path: str = "indifferent",
direct_path_mode: Optional[Sequence[str]] = None,
add_poi_infos: Sequence[str] = [],
add_poi_infos: Sequence[str] = "none",
Copy link
Owner

Choose a reason for hiding this comment

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

Dito

jonperron added a commit that referenced this pull request Jun 15, 2024
@jonperron
fix(journey): fix support of add_poi_infos
d3f0c7c 
Fix issue in PR #63

* Add arg as optionnal (see documentation)
* Update docs
* Add tests to check URL
@jonperron jonperron mentioned this pull request Jun 15, 2024
Merged
jonperron added a commit that referenced this pull request Jun 15, 2024
@jonperron
fix(journey): fix support of add_poi_infos
04d5f8d 
Fix issue described in PR #63

* Add arg as optional (see documentation)
* Update docs
* Add tests to check URL
jonperron added a commit that referenced this pull request Jun 15, 2024
@jonperron
fix(journey): fix support of add_poi_infos (#67)
7ce205c 
Fix issue described in PR #63

* Add arg as optional (see documentation)
* Update docs
* Add tests to check URL
@jonperron
Copy link
Owner

Should be fixed for this endpoint + handling of sequences has been improved in PR #67 and #64

@jonperron jonperron closed this Jun 15, 2024
@jonperron
Copy link
Owner

Available in version 1.1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonperron jonperron jonperron requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SemiCroustii @jonperron