Skip to content

Conversation

@zulianrizki
Copy link

this is my PR for #26

there is no breaking change.
just use timetable.setInterval(interval) in minute to set time interval, default value is 60 or 1 hour.

Sorry if my code is messy. this is my first github contribution.
Thanks !

@toadkicker
Copy link

Hey I looked at this and just wanted to just add some advice though.

  1. It's hard to read the diff because your editor formatted the code differently than the original, making every line seem like part of the change. If you're going to do that (not a good idea though) its best to have a commit that is only related to formatting code. Maybe even make it a separate PR. While I would agree your formatting is what most javascript development is using, it just makes diffing harder and distracts from the goals.

  2. I wouldn't call this method setInterval, although that is a good name. The issue is this is already reserved in Javascript to mean something different, and while there is little chance they could collide developers would be confused as to the intent of the implementation. Perhaps just call it displayInterval?

  3. Some developers must be using Visual Studio, and that's why settings.json is there. Deleting it doesn't have anything to do with the goal of this PR, and its probably helpful to those developers.

  4. Use git squash and clean up your commits to as few as possible.

I'm planning to use this project in a few days for something I'm working on so I happened to be checking out what was outstanding and this feature is important to me too.

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