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

[tech] make some items public #838

Merged
merged 10 commits into from
Jan 25, 2022
Merged

Conversation

patochectp
Copy link
Member

No description provided.

Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

There is a bunch of new exposed API which are not hidden behind the feature-flag gtfs_parser:

  • gtfs::EquipmentList (which was in gtfs::read::EquipmentList before and gtfs::read is private)
  • calendars::manage_calendars (which was pub(crate) but is now pub and module calendars is pub)
  • read_utils::FileHandler (and friends read_utils::PathFileHandler and read_utils::ZipFileHandler) which I really would not expose, at least not without a feature (another file_handler feature?)
  • read_utils::read_objects, read_utils::read_collection and read_opt_collection, not a fan of exposing those either, but it doesn't make sense to hide it behind gtfs_parser or file_handler features

Idea - maybe it's not a gtfs_parser feature that we need but 2 features: gtfs and parser. You would hide all of gtfs::read behind #[cfg(all(feature = "gtfs", feature = "parser"))] and hide the read_* functions of read_utils behind #[cfg(feature = "parser")]?

@patochectp patochectp requested a review from woshilapin January 18, 2022 11:30
@patochectp patochectp requested a review from woshilapin January 18, 2022 15:20
@patochectp patochectp requested a review from woshilapin January 18, 2022 16:35
@patochectp patochectp requested a review from woshilapin January 19, 2022 10:20
woshilapin
woshilapin previously approved these changes Jan 20, 2022
@woshilapin
Copy link
Contributor

I was thinking it would be nice to add some documentation about the 2 new introduced features into the preamble of src/lib.rs (see how it's already done for the existing features).

@patochectp
Copy link
Member Author

I was thinking it would be nice to add some documentation about the 2 new introduced features into the preamble of src/lib.rs (see how it's already done for the existing features).

I told myself the same thing

src/lib.rs Outdated
@@ -38,6 +38,13 @@
//! mutate a `Model`. It might not be completely stable at the moment so use
//! with care (or not at all!).
//!
//! ## `gtfs`
//! This feature is only used to expose some gtfs functions for use in external projects
Copy link
Contributor

Choose a reason for hiding this comment

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

I would mention that the exposed API are not stable and that using this is experimental. I'm not expecting us to be as careful about the stability here that for the rest of the exposed API. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

done in 6f11bef

woshilapin
woshilapin previously approved these changes Jan 21, 2022
@patochectp patochectp force-pushed the gtfs_makes_some_items_visible branch from 41f1c18 to d18eb0d Compare January 24, 2022 09:47
@patochectp patochectp merged commit 140f861 into master Jan 25, 2022
@patochectp patochectp deleted the gtfs_makes_some_items_visible branch January 25, 2022 09:44
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