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

Version 4 update #15

Merged
merged 33 commits into from
Apr 12, 2024
Merged

Version 4 update #15

merged 33 commits into from
Apr 12, 2024

Conversation

SamJaarsma
Copy link
Contributor

Several improvements from reviews:

  • Split the travellers message into a basic message used for search and an extended message for confirming the booking
  • Add a disclaimer for licensed code-types like GiataID and DRV GlobalTypes
  • Better explain the "unspecified" use in enums
  • Activity message: reorganized/cleaned-up to use just 3 levels: product_code, unit_code and service_code. Added proper explanation
  • Added on-request/available status in search results
  • Transport message: removed trip departure and arrival as it can be derrived from the first and last segment to avoid contradictions
  • Explained sequence number logic for transport message
  • All ProductList messages: made product_status and enum
  • Added missing last modified timestamp to ActivityProductList and ActivityProductInfo
  • Improved version explanation
  • Transport message move servicefact to TripSegment
  • Transport message include change policies
  • Improve explanation for SearchResponseMetadata metadata, context
  • Review multi room explanation
  • Review currency message to allow for crypto, create string and specify number of decimals in an int32 (between 2 and 18)

@havan
Copy link
Member

havan commented Apr 3, 2024

I will update the code and push to this PR the changes that are simple fixes, typos etc to save time.

Copy link
Member

@havan havan left a comment

Choose a reason for hiding this comment

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

I've added multiple comments though out the changes.

proto/cmp/services/transport/v1alpha/search.proto Outdated Show resolved Hide resolved
proto/cmp/services/activity/v1alpha/search.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/currency.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/currency.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/currency.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/currency.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/traveller.proto Outdated Show resolved Hide resolved
@havan
Copy link
Member

havan commented Apr 4, 2024

specify number of decimals in an int32 (between 2 and 18)

Why between 2 and 18? It can be any positive number and also it can be zero. Am I missing something?
(Well, it can be negative numbers also, math works but it's confusing)

@SamJaarsma
Copy link
Contributor Author

specify number of decimals in an int32 (between 2 and 18)

Why between 2 and 18? It can be any positive number and also it can be zero. Am I missing something? (Well, it can be negative numbers also, math works but it's confusing)

changed to 0-18

proto/cmp/services/accommodation/v1alpha/search.proto Outdated Show resolved Hide resolved
proto/cmp/services/transport/v1alpha/search.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/search.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/search.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/service_fact.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/service_fact.proto Outdated Show resolved Hide resolved
proto/cmp/types/v1alpha/traveller.proto Outdated Show resolved Hide resolved
Copy link
Member

@havan havan left a comment

Choose a reason for hiding this comment

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

LGTM

@havan havan merged commit 3db62ed into dev Apr 12, 2024
3 checks passed
@havan havan deleted the version-4-update branch April 12, 2024 09:28
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