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

Added data structure for exposing country descriptions #121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cweitkamp
Copy link
Contributor

@cweitkamp cweitkamp commented Oct 29, 2019

For discussion:

  • Added base classes for Countries, Regions and Cities.
  • Added prototype methods for requesting available countries, regions and cities.
  • The parsing - and optionally caching - could be improved. I just want to know if this is something which is in line with your library.

As a side note: May formatter is different than yours. Bear with me if this does not comply with your guidelines.

// EDIT: I just found your code-formatter.xml ...

Closes #110

Signed-off-by: Christoph Weitkamp [email protected]

@coveralls
Copy link

coveralls commented Oct 29, 2019

Coverage Status

Coverage decreased (-0.8%) to 63.379% when pulling cb566db on cweitkamp:feature-110-country-description-provider into fcc9b8f on svendiedrichsen:master.

@cweitkamp cweitkamp force-pushed the feature-110-country-description-provider branch from c4e14e6 to 8a9f552 Compare October 29, 2019 15:14
@cweitkamp cweitkamp force-pushed the feature-110-country-description-provider branch from f3782ec to cb566db Compare October 31, 2019 20:49
@cweitkamp cweitkamp marked this pull request as ready for review October 31, 2019 20:53
@svendiedrichsen
Copy link
Owner

@cweitkamp Thank you for your effort but this API is in my view only about handling holiday calendars. Those calendars are structuring holidays hierarchically without interpreting those hierarchies. They can be (and are mainly, looking at the provided ones) countries. But even the provided calendars contain examples like Holidays_nsye_euronext.xml which contain holidays of the NYSE. Generally should it be possible to have any kind of hierarchical structure of holidays like i.e.

  • one for an international company and its national sub-companies
  • or a company and some form of company holidays by company branches
  • a calendar of all stock exchanges worldwide
  • some kind of personal calendar of all family members

I admit that I have also started thinking only about countries first but since I have seen several different kind of holiday calendar structures.

@cweitkamp
Copy link
Contributor Author

@svendiedrichsen Thank you for your opinion. You are absolutely right. To take care of the country structure is not the purpose of this API.

For the requirements specified in #110 it would be sufficient to expose the resource bundles of country descriptions. Would it be okay for you to change the visibility of the methods ResourceUtil::getCountryDescriptions() and ResourceUtil::getHolidayDescriptions() to public? The rest can be implemented on our side.

private ResourceBundle getCountryDescriptions(Locale l) {
return getResourceBundle(l, COUNTRY_DESCRIPTIONS_CACHE, COUNTRY_DESCRIPTIONS_FILE_PREFIX);
}

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.

Feature Request: Expose country descriptions for external usage
3 participants