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

ExIcal is locked to an old version of Timex #8

Closed
leesharma opened this issue Aug 23, 2016 · 2 comments
Closed

ExIcal is locked to an old version of Timex #8

leesharma opened this issue Aug 23, 2016 · 2 comments

Comments

@leesharma
Copy link
Contributor

Current Behavior

ex_ical is locked on to an old release of timex ({:timex, "~> 1.0"}, they're currently at 3.x).

This means that in order to use ex_ical in a project (assuming that the project isn't using timex ~> 1.0, a developer needs to not only include an extra library/version, but they will also need to write their own adapter from the old Timex datetime formats into whatever library they're using in their application.

Desired Behavior

This library should be able to work with current DateTime libraries.

Since Date and DateTime were only just added to Elixir (end even then, they aren't a complete implementation), there are a number of libraries available (timex, calendar, good_times, etc.) Ideally, using ex_ical shouldn't lock you into having to use a specific version of a specific library–it should be able to work with a number of options. Just upgrading to timex ~> 3.0 makes the situation better, but it won't solve the full problem. I think that using an adapter pattern and letting the user specify their library/adapter is the best way to handle this.

Possible Interface

Ideally, something like the following interface would work. It should be able to handle a default case with minimal boilerplate required; common cases with a small amount of configuration; and "unsupported" cases with a bit more configuration.

Using the default datetime library (Timex, current version)

# mix.exs

def applications do
  [applications: [:timex, ...]]
end

def deps do
  [
    {:timex, "~> 3.0"},
    {:ex_ical, "~> 1.0"},
    ...
  ]
end

Using calendar, another popular DateTime library

# mix.exs

def applications do
  [applications: [:calendar, ...]]
end

def deps do
  [
    {:calendar, "~> 1.0"},
    {:ex_ical, "~> 1.0"},
    ....
  ]
end


# config/config.exs

config :ex_ical,
  datetime_library: :calendar

Using an unsupported or "bespoke" DateTime solution

# mix.exs

def applications do
  [applications: [...]]
end

def deps do
  [{:ex_ical, "~> 1.0"}, ...]
end


# config/config.exs

config :ex_ical,
  datetime_library: :bespoke_datetime,
  datetime_adapter: BespokeDateTime.Adapter  # the user writes this module
@fazibear
Copy link
Owner

It's a great idea. Didn't notices that they updating Timex so fast. Anyway I think this is a great solution. We will not depend on specific library. It will fit to any project. We can support interfaces for most popular libraries.

@leesharma
Copy link
Contributor Author

Yay! 😄

I think that using mix config probably makes the most sense for this. Since project config supersedes app config, we could set defaults in-app and parent project would be able to override them. This setup should be pretty testable too–we can manually set the config with Application.put_env/4 and test the adapter interface against all built-in options.

For an example, Ecto uses almost the same pattern: they have Application.get_env/3 and a case statement to select the adapter class in tests (though they use env vars) and a set of functions/macros to get the class in lib (some example usage here: Ecto.Adapters.SQL, case statement in tests).

Maybe starting with a fixed set of adapters for the most common cases would be easiest (maybe the current versions on timex and calendar)? After that, the option to add custom adapters can be added.

leesharma added a commit to leesharma/ex_ical that referenced this issue Aug 25, 2016
`import` imports all of a module's functions into the local namespace.
Using `alias` instead gives finer control over what exists in the
namespace.

Additionally, this change makes it clearer how we're using `Timex`. When
we go to refactor that out (see fazibear#8), this should make it a bit easier.
fazibear pushed a commit that referenced this issue Jun 7, 2017
* update credo

Credo was showing a warning message, so I upgraded. This shouldn't
affect behavior or warnings.

* make `ExIcal.Utils` functions private

These two functions: `date_before?/2` and `date_after?/2`, are only used
internally in the `ExIcal.Utils` module. They aren't delegated to the
`ExIcal` module, and their API (`ExIcal.Utils.date_before/2`, etc)
suggests that we don't expect developers to use that API.

Making these two functions private until a compelling reason is given to
publish them means that we can change the internal API with a breaking
change, and it's less public interface to maintain. It'll be easier to
make these public later on than to hide them.

* use `alias` instead of `import`

`import` imports all of a module's functions into the local namespace.
Using `alias` instead gives finer control over what exists in the
namespace.

Additionally, this change makes it clearer how we're using `Timex`. When
we go to refactor that out (see #8), this should make it a bit easier.

* add typespecs

The API is stable-ish, so I added typespecs. This means that the `s`
command in iex will list the public methods, and the generated exdocs
will be more detailed.

Many of these specs do use Timex-derived structs (`%Timex.DateTime{}`),
so in the course of #9, these will need to be changed.

* consolidate aliases

The code was a bit inconsistent in which aliases were declared, and some
submodules under the same module were declared on multiple lines (as
opposed to the `ExIcal.{SubOne,SubTwo}` syntax).

Now all the `Timex` and `ExIcal` repeated use aliases are declared at
the top of the file, and they all use the curly bracket notation for
consistency. I hope that this makes it easy to see the major file
dependencies in one place as well as makes the code more concise.

* flesh out documentation

This commit adds/fleshes out documentation in all modules and public
interface functions. My goal here was to make the library look more
"polished" and to help users find answers to their questions related to
`ExIcal` or the iCal spec. I tested it out in `iex` as well as the
generated docs, and everything looks as intended.

Changes:

  - add/improve lots of documentation
  - tidy code in `lib/ex_ical/utils` (break up long line)

Decisions & Justifications:

  - **lots of links**: in `lib/ex_ical/event.ex`, I included a lot of links
    to the iCal spec. They render as expected in the generated docs, but
    they show up as an ugly-ish list in `iex`. I think it's still worth
    it to have them since they'll help people with questions about the
    iCal property rules.

  - **ask for input**: in `lib/ex_ical/event.ex` and
    `lib/ex_ical/parser.ex`, I included paragraphs mentioning that the
    spec was not fully implemented and to file an issue if they need
    some unsupported property. I think it's valuable to have this in the
    docs because that's where people will go if they're looking for
    property support, but it might be too conversational for your taste.
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

No branches or pull requests

2 participants