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

Add new fare_action enum options to fare_transactions #122

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

jaygordon
Copy link
Contributor

@jaygordon jaygordon commented Apr 6, 2023

Added the new actions discussed in the linked issue.

Pull Request

Before submitting a pull request, please read CONTRIBUTING.md.

For spec changes:

  • Pull-requests must address an existing issue
  • Update relevant documentation.

By contributing to this project, all contributors certify to the Developer Certificate of Origin in CONTRIBUTING.md.

Added the new actions discussed in the linked issue.
@jaygordon jaygordon linked an issue Apr 6, 2023 that may be closed by this pull request
@botanize botanize changed the title Update fare_transactions.schema.json Add new fare_action enum options to fare_transactions Apr 7, 2023
@jaygordon jaygordon self-assigned this Apr 19, 2023
Copy link
Contributor

@botanize botanize left a comment

Choose a reason for hiding this comment

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

Jay, thank you for this pull-request. I think this looks great, but I'm concerned that the enum values themselves aren't quite self-documenting. If I hadn't read the linked issue I'm not sure what I would have interpreted "Capture" to mean. I see a few options,

  1. This is industry standard vocabulary and additional description isn't necessary
  2. The enum values could be longer to provide a succinct description, e.g., Button pressed by driver or operator to indicate a boarding or alighting passenger from fare_transactions.media_type.
  3. Leave the enum values the same and add descriptions of them to the description property.

@gabriel-korbato
Copy link
Contributor

I like the third option the most. Even if it is industry-standard, some may not be sure what it means. I don't like long enum values because they bloat data files. (At Korbato we use numeric IDs pointing to an enum table.)

@e-lo e-lo requested a review from botanize June 14, 2023 18:38
@jaygordon
Copy link
Contributor Author

I prefer the third option too.

@jaygordon
Copy link
Contributor Author

If we're in agreement I can implement option 3 and add the id and description of each enum value in the description field. I think newlines and tabs would be useful here but I'm not sure how that's parsed. Should I do a single escape (\n) or will a parser require a double escape (\\n)?

@jlstpaul
Copy link
Contributor

Are we still waiting on response to how to add newlines and tabs to the description field? @e-lo @j-meelah @SorenSpicknall do you have any thoughts on that?

@e-lo
Copy link
Contributor

e-lo commented Jul 11, 2023

Re Enums...

I like Option 3 and use it in other projects:

  1. Leave the enum values the same and add descriptions of them to the description property.

Getting it to render a newline might take trial and error, but I believe that HTML <br> is most likely the winner since it is available in Markdown which is how we are rendering things.

<br>- Value 1<br>- Value 2<br>- Value 3

Another option would be to make an unordered list, which I've done before too.

@e-lo
Copy link
Contributor

e-lo commented Jul 11, 2023

And, while I believe that self-documenting within the schema is the right way to go, there is the option to define an RDF type in a separate json-file which could be self-documenting....but gross IMHO.

@e-lo
Copy link
Contributor

e-lo commented Jul 11, 2023

Also... submitted frictionlessdata/datapackage#843 for people to add their own thoughts to

Added enum descriptions, trying html line breaks
@github-actions
Copy link
Contributor

Documentation available at: http://tides-transit.github.io/TIDES/108-add-other-fare_actions

@SorenSpicknall SorenSpicknall merged commit 314f4aa into main Aug 9, 2023
@e-lo e-lo deleted the 108-add-other-fare_actions branch January 6, 2024 15:57
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.

📄🚀 Add other fare_actions
6 participants