✨ GoingToCamp Provider 🏕

[acaloiaro]

This PR adds Going to Camp as a camply provider.

Addresses #94

[acaloiaro]

A few items not yet noted by TODO in the code.

[acaloiaro]

Support weekends only?

[juftin]

The BaseCampingSearch base class should handle all of this heavy lifting.

Here's how it works inside with Recreation.Gov in the final get_all_campsites abstractmethod:

camply/camply/search/search_recreationdotgov.py

Lines 247 to 257 in d1fb5d5

	campsite_df = self.campsites_to_df(campsites=found_campsites)
	campsite_df_validated = self._filter_date_overlap(campsites=campsite_df)
	compiled_campsite_df = self._consolidate_campsites(
	campsite_df=campsite_df_validated, nights=self.nights
	)
	equipment_filtered_campsites = self.filter_campsites_to_equipment(
	campsites=compiled_campsite_df
	)
	compiled_campsites = self.df_to_campsites(
	campsite_df=equipment_filtered_campsites
	)

The idea is that found_campsites array is a list of AvailableCampsite objects which may or may not fit the search criteria. Every AvailableCampsite has a booking nights length of 1 and the _filter_date_overlap will take care of filtering to match dates (including weekends) and _consolidate_campsites will handle combining consecutive nights together for the same campsite/campground ID when the user specifies that nights > 1.

Yellowstone was a bit different than this since it allowed you to forward the nights param directly to the API to only return bookings matching that.

[acaloiaro]

The current state of this PR Is that, due to each rec area having its own naming conventions for equipment, equipment matching needs to be improved.

[acaloiaro]

@juftin What are your thoughts on the added complexity of having both --equipment and --equipment-id?

--equipment-id is the easiest way forward to support Going To Camp's native equipment type support, in lieu of the (equipment, length) tuple that powers Rec.gov and Yellowstone. I'm not in love with having both, and equally not in love with overloading --equipment to support both rec.gov and goingtocamp.

[acaloiaro]

One middle ground alternative might be to break the --equipment API up into --equipment and --equipment-length (default 0, although I think -1 is more fitting sentinel value).

After breaking up the --equipment API it could be overloaded such that --equipment takes an equipment name, i.e. trailer, tent, etc -- or an equipment ID, for providers that support specific equipment out of the box.

[juftin]

I'm open to breaking it up. My idea with --equipment accepting a tuple of equipment-name and equipment-length is that it would allow you to specify for multiple types of equipment, with different lengths:

camply campsites \
    --rec-area 2991 \
    --start-date 2022-07-09 \
    --end-date 2022-07-17 \
    --nights 5 \
    --equipment RV 25 \
    --trailer 15

But now that I'm thinking about it - maybe that's not a common scenario and specifying a single equipment name (or id) and a single equipment length is more useful.

[acaloiaro]

👍 I'll take a stab at implementing them as two params and a single equipment-per-search. Seems like a reasonable middle ground.

[juftin]

Hey @acaloiaro feedback is coming soon, I promise. I'm a little preoccupied with some work and personal things at the moment but I'll be following up here as soon as I can

[acaloiaro]

@juftin This PR is ready for a proper review.

Notable changes since you may have last seen it

• --equipment and --equipment-length are separate.
• I moved --provider on to the top-level of the CLI. Maybe this is too much of a breaking change. Happy to revert it if you think it doesn't make sense.

[juftin]

👏 👏 @acaloiaro thank you so much - I'm starting to look at this on my end. I've got some big life events this week (🤵 💍 👰‍♀️ ) and so review will be delayed.

One question on the --provider change:

• I love moving --provider to the top level of the CLI. One question though, does this mean that passing it in after --campsites will break? I really like options like --profile in the AWS CLI where you can just throw it in wherever. I'm wondering if Click gives us any kind of flexibility on those universal options like --provider

[acaloiaro]

In its current implementation, I think it does break that behavior, but let me see if click will allow the alternate behavior at the same time.

[acaloiaro]

BTW I still need to add some tests and manually test more things, like notifications and configuration file support, before this PR will actually be ready for consideration. I just wanted to get another set of eyes on it for now.

[juftin]

Hey @acaloiaro following up here. I've actually moved --provider and --debug to the top level and lower level CLI namespaces via some Click Contexts on Release 0.7.0.

I've also added a Discord Server - it's pretty much empty but I wanted to make sure there's an easy place to collaborate for features like yours. Is there anything I can do to help you along on this right now?

[acaloiaro]

Cool! I'm taking a look at this PR again now and will try to get it polished up.

[juftin]

Validators use cls instead of self - kind of confusing but that's what's throwing the error here: https://github.com/juftin/camply/actions/runs/3576943050/jobs/6015349570#step:8:427

I like the idea of validating the start date at the SearchWindow level - currently I coerce the SearchWindow into being compliant with the current date here:

camply/camply/search/base_search.py

Lines 542 to 569 in 173ee47

	def _get_search_days(self) -> List[datetime]:
	"""
	Retrieve Specific Days to Search For
	Returns
	-------
	search_days: List[datetime]
	Datetime days to search for reservations
	"""
	current_date = datetime.now().date()
	search_nights = set()
	for window in self.search_window:
	generated_dates = {
	date for date in window.get_date_range() if date >= current_date
	}
	search_nights.update(generated_dates)
	if self.weekends_only is True:
	logger.info("Limiting Search of Campgrounds to Weekend Availabilities")
	search_nights = {x for x in search_nights if x.weekday() in [4, 5]}
	if len(search_nights) > 0:
	logger.info(
	f"{len(search_nights)} booking nights selected for search, "
	f"ranging from {min(search_nights)} to {max(search_nights)}"
	)
	else:
	logger.info(SearchConfig.ERROR_MESSAGE)
	raise RuntimeError(SearchConfig.ERROR_MESSAGE)
	return list(sorted(search_nights))

If we need to set this data validation at the SearchWindow level these validators should coerce the dates into >= current date instead of raising an Error.

I kind of like the idea of being able to provide a start date in the past as long as the end date is in the future without raising an error. What do you think?

[acaloiaro]

Yeah that makes sense; I'd prefer it to be more tolerant of bad input.

[juftin]

Nice - I wish I could easily remember exactly where my datetime objects get converted into Timestamps and then become difficult to deal with.

[acaloiaro]

I originally I remember intending to track that down and factoring this code out. I vaguely recall this having something to do with dates coming from a config file? Does that ring any bells?

[juftin]

I think there are a couple places where I convert an array of AvailableCampsite objects into a pandas DataFrame, do some manipulation, and then transform them back into an array of AvailableCampsite - somehow tunring them into Timestamp along the way. This might be the big offender here:

camply/camply/search/base_search.py

Lines 752 to 769 in 173ee47

	@staticmethod
	def df_to_campsites(campsite_df: DataFrame) -> List[AvailableCampsite]:
	"""
	Convert Campsite DataFrame to array of AvailableCampsite objects
	Parameters
	----------
	campsite_df: DataFrame
	Returns
	-------
	List[AvailableCampsite]
	"""
	composed_campsite_array = list()
	composed_campsite_data_array = campsite_df.to_dict(orient="records")
	for campsite_record in composed_campsite_data_array:
	composed_campsite_array.append(AvailableCampsite(**campsite_record))
	return composed_campsite_array

[juftin]

This is all looking very clean - it's a great implementation. I've got a couple questions on here but I'm close to getting this ready for merge.

There's currently a GitHub Actions Secrets error that occurs when I merge directly to main from an outside repo - let's target a different branch on juftin/camply and then I'll go to main from there.

I'd like to get some usage of GoingToCamp documented - including how to use the new equipment-types command. I've got a placeholder here: #156. If it's helpful we can merge this PR into that branch or merge both PRs into a new branch.

[acaloiaro]

Cool, let's make #156 the target for this branch to address both issues. And I'll take a look at #156 to see about adding more equipment-type documentation.

[acaloiaro]

This smells. It looks like we should be iterating over all search windows. I wasn't aware that multiple search windows were a valid use case.

[juftin]

There are also a few of properties set on the BaseCampingSearch instance that might be useful:

camply/camply/search/base_search.py

Lines 90 to 92 in 173ee47

	self.search_days: List[datetime] = self._get_search_days()
	self.search_months: List[datetime] = self._get_search_months()
	self.nights = self._validate_consecutive_nights(nights=nights)

• self.search_dates = An array of all Possible Dates from Among Your Search Window that are today or in the future
• self.search_months = An array of all of the months that comprise the search window expressed as a first of the month dt object
• self.search_nights = The number of nights to search - this is also validated so if you tell camply that you want to search for weekends only but you want to stay for 6 nights, it's adjusted to 5 - or if your search window is 3 days wide but you ask for 4 nights

[acaloiaro]

Cool, let's make #156 the target for this branch to address both issues. And I'll take a look at #156 to see about adding more equipment-type documentation.

[juftin]

Nice, these are great. Thank you so much for the testing contributions.

[juftin]

I feel great with all of this - I'll leave it up to you for when you're fully happy with merging.

I'm guessing that currently I only have the permissions to hit those buttons. How should that merge go, any preferences? For me, ideally the final merge into main is single clean commit belonging to you.

[juftin]

I'm going to move forward and get this code staged to go to main. It's going to an intermediate release branch in the meantime where I can finally bring my testing changes up to date with it. 🚀