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

Multiple days #15

Closed
wants to merge 2 commits into from
Closed

Multiple days #15

wants to merge 2 commits into from

Conversation

manuel-rubio
Copy link

And closes #13 and #14.

@jbernardo95
Copy link
Owner

jbernardo95 commented May 16, 2018

@manuel-rubio First of all thank you so much for the contribution ! 🙌

Now going into the pr:

  1. I see that the issue title is "Multiple days", but this does not tell me much, can you please elaborate on this ? Can you tell me what you are trying to add, with some examples ?
  2. I quickly looked at the code and saw that there are no tests, can you please add them ? (Tests are a good way to express what is the intent of what you added).
  3. I see that you included fixes for Update minimum Elixir version to 1.7 #13 and In Elixir 1.6.4 timing is not correct #14 which is awesome ! But, to make sure changes are isolated from one another, can you please open a new pr for each one of these fixes ?

If any of this was not clear or you need help with anything, please tell me so I can help you 🙂Thanks again !

@manuel-rubio
Copy link
Author

@jbernardo95 actually I found the commits from @paweljw and I was interested mainly in the fix to run cronex every minute and not every 30 seconds.

Reading the code I guess it's letting to you to add more than one day in the configuration, but not completely sure, @paweljw could you explain or give us an example about how to use your modification?

@paweljw
Copy link

paweljw commented May 17, 2018

We're using it like this:

every [:monday, :tuesday, :wednesday, :thursday, :friday], at: "06:00" do
# ...
end

That being said, it's a spur-of-the-moment fork and patch; the code is not documented, not covered with tests, and consists mostly of copypasta. I would not recommend merging it into master as-is, and as such I'm slightly confused why this pull request even exists :)

However, if you want to fork my fork, fix the issues listed above and open a PR from your fork, go for it :)

@manuel-rubio
Copy link
Author

ok, sorry @paweljw , closing the PR.

@jbernardo95
Copy link
Owner

I see. Makes sense to me !

I opened an issue for this (#17).

@manuel-rubio If you want to open a pr for this you are more than welcome. I will try to fix the 30 seconds issue and the migration to elixir 1.6 in the following days.

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.

Update minimum Elixir version to 1.7
3 participants