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

Rename weekday to workweek #313

Merged
merged 3 commits into from
Nov 11, 2017
Merged

Conversation

kaphacius
Copy link
Member

@kaphacius kaphacius commented Nov 10, 2017

Checklist

As per this wikipedia page

Another variant is isAWorkday or isAWeekday, but I think isInWorkweek works better with isInWeekend

@SwifterSwiftBot
Copy link

SwifterSwiftBot commented Nov 10, 2017

2 Warnings
⚠️ Consider adding tests for new extensions or updating existing tests for a modified SwifterSwift extension
⚠️ The source files have been modified. Please consider adding a CHANGELOG entry if necessary.
1 Message
📖 Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.

Generated by 🚫 Danger

@codecov
Copy link

codecov bot commented Nov 10, 2017

Codecov Report

Merging #313 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #313      +/-   ##
==========================================
+ Coverage   94.63%   94.63%   +<.01%     
==========================================
  Files         100      102       +2     
  Lines        5780     5800      +20     
==========================================
+ Hits         5470     5489      +19     
- Misses        310      311       +1
Flag Coverage Δ
#ios 94.43% <100%> (ø) ⬆️
#osx 61.63% <100%> (+0.11%) ⬆️
#tvos 89.61% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
Sources/Extensions/Foundation/DateExtensions.swift 100% <100%> (ø) ⬆️
Tests/FoundationTests/DateExtensionsTests.swift 95.27% <100%> (ø) ⬆️
...ces/Extensions/Foundation/CalendarExtensions.swift 50% <0%> (ø)
Tests/FoundationTests/CalendarExtensionTest.swift 100% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46a5d8e...4b8d205. Read the comment docs.

Copy link
Member

@SD10 SD10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kaphacius, thank you for submitting this PR but I disagree with changing the name on this one. I believe weekday is the proper terminology

@omaralbeik
Copy link
Member

Thank you @kaphacius, as mentioned in Apple's documentation, weekday refers to any day in the week, including weekend, which might be confusing here.

Personally I've never heard the word workweek before, but English is not my main language 😃
We might also consider renaming it to isWorkDay or isBusinessDay, what do you think guys?

@SD10
Copy link
Member

SD10 commented Nov 10, 2017

I see, after looking at the documentation I wouldn't be opposed to isWorkday

@kaphacius
Copy link
Member Author

done, isWorkday it is :)

@@ -275,9 +275,9 @@ final class DateExtensionsTests: XCTestCase {
XCTAssertEqual(date.isInWeekend, Calendar.current.isDateInWeekend(date))
}

func testIsInWeekday() {
func testisWorkday() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaphacius Could you please rename this to testIsWorkday to keep consistency with other tests :)

@omaralbeik omaralbeik merged commit e92a82e into SwifterSwift:master Nov 11, 2017
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.

4 participants