Skip to content

SeatContext proposal#909

Merged
Aurige merged 20 commits intonextfrom
seatcontext_proposal
Jun 3, 2025
Merged

SeatContext proposal#909
Aurige merged 20 commits intonextfrom
seatcontext_proposal

Conversation

@TuThoThai
Copy link
Copy Markdown
Contributor

Addresses both issue #701 and group decision made on 18 March 2025 in PR #813

skinkie added 2 commits May 27, 2025 08:16
…nce to keep deprecated structures before there was even a release.
@skinkie skinkie added the hygiene Technical dept, results in a breaking change. label May 27, 2025
@skinkie skinkie self-assigned this May 27, 2025
skinkie
skinkie previously approved these changes May 27, 2025
@skinkie skinkie requested a review from JohanEntur May 27, 2025 06:24
Aurige
Aurige previously approved these changes May 27, 2025
@Aurige Aurige added this to the netex_2.0 milestone May 27, 2025
@Aurige Aurige added the needs documentation update The NeTEx document needs to be updated label May 27, 2025
ue71603
ue71603 previously approved these changes May 27, 2025
@TuThoThai TuThoThai linked an issue May 27, 2025 that may be closed by this pull request
Comment thread xsd/netex_framework/netex_reusableComponents/netex_seatingPlan_version.xsd Outdated
Comment thread xsd/netex_framework/netex_reusableComponents/netex_seatingPlan_version.xsd Outdated
@skinkie skinkie dismissed stale reviews from Aurige and themself via cb62fa9 May 27, 2025 09:29
Comment thread xsd/netex_framework/netex_reusableComponents/netex_seatingPlan_version.xsd Outdated
Changes are: ByAisle -> IsByAisle, ByWindows -> IsByWindows, BetweenSeats -> IsBetweenSeats
@Robbendebiene
Copy link
Copy Markdown
Contributor

I'm still not sure about the arguments against the enum.

Because it is far more difficult to parse.

@skinkie Out of curiosity, what makes it difficult to parse? Since basically every facility is a SomeEnumListOfEnumerations are they hard to parse as well? I can see that boolean properties translate easily into boolean variables of a class but shouldn't a SomeEnumListOfEnumerations also easily translate into a single variable of type List<SomeEnum>?

@Robbendebiene
Copy link
Copy Markdown
Contributor

Last suggestion from my side 😇
If HasWindowInFront and HasAisleInFront should be kept then perhaps turning them into the same format as the other booleans (IsABC) makes sense to "artificially" group them together?

  • HasWindowInFront -> IsFacingWindow or IsBeforeWindow
  • HasAisleInFront -> IsFacingAisle or IsBeforeAisle

@skinkie
Copy link
Copy Markdown
Contributor

skinkie commented Jun 2, 2025

@skinkie Out of curiosity, what makes it difficult to parse? Since basically every facility is a SomeEnumListOfEnumerations are they hard to parse as well? I can see that boolean properties translate easily into boolean variables of a class but shouldn't a SomeEnumListOfEnumerations also easily translate into a single variable of type List<SomeEnum>?

The biggest problem is that the changes made by Nick basically combined variants (permutation) in single instances, this is what we are reverting back to direct class attributes. SomeEnumeListOfEnumerations leads to complex queries for OR and AND operators, not directly available without explict parsing and handeling of that list with IN like queries, where the checks have to be explicitly modelled based on the enumeration.

@TuThoThai
Copy link
Copy Markdown
Contributor Author

Last suggestion from my side 😇 If HasWindowInFront and HasAisleInFront should be kept then perhaps turning them into the same format as the other booleans (IsABC) makes sense to "artificially" group them together?

* `HasWindowInFront` -> `IsFacingWindow` or `IsBeforeWindow`

* `HasAisleInFront` -> `IsFacingAisle` or `IsBeforeAisle`

Works for me. I likeIsFacingWindow and IsFacingAisle better as they sound clearer to me. Updates on the PR coming!

@TuThoThai TuThoThai requested review from Aurige, skinkie and ue71603 and removed request for JohanEntur June 2, 2025 18:52
skinkie
skinkie previously approved these changes Jun 2, 2025
Comment thread examples/functions/deckPlans/DeckPlans-Example_train_simple.xml Outdated
Comment thread examples/functions/deckPlans/DeckPlans-Example_train_simple.xml Outdated
@skinkie
Copy link
Copy Markdown
Contributor

skinkie commented Jun 2, 2025

@TuThoThai some more validation errors...

@TuThoThai
Copy link
Copy Markdown
Contributor Author

@TuThoThai some more validation errors...

@skinkie, apologies... my eyes were clearly not working properly! Thanks for fixing the errors!

@Robbendebiene
Copy link
Copy Markdown
Contributor

Would it make sense to default all of these properties to false? I'm just asking as I'm not sure if there are any conventions in NeTEx when to use default values and when not.

@skinkie
Copy link
Copy Markdown
Contributor

skinkie commented Jun 3, 2025

@Robbendebiene yes good to mention it. Sane defaults is another issue, better do it directly correct.

@TuThoThai
Copy link
Copy Markdown
Contributor Author

@skinkie and @Robbendebiene, default value is now false for all these attributes

@TuThoThai TuThoThai requested a review from skinkie June 3, 2025 09:40
@skinkie
Copy link
Copy Markdown
Contributor

skinkie commented Jun 3, 2025

@Robbendebiene yes good to mention it. Sane defaults is another issue, better do it directly correct.

We did the same, but I did not push yet.

@Aurige Aurige merged commit 5cd2b73 into next Jun 3, 2025
1 check passed
@trurlurl trurlurl added document has been updated NeTEx Document already updated and removed needs documentation update The NeTEx document needs to be updated labels Jun 4, 2025
@TuThoThai TuThoThai deleted the seatcontext_proposal branch February 17, 2026 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

document has been updated NeTEx Document already updated hygiene Technical dept, results in a breaking change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SeatContext should be boolean or a list of values

7 participants