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

proposal: time: export time.isLeap #20944

Closed
petrus-v opened this issue Jul 7, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@petrus-v
Copy link

commented Jul 7, 2017

What version of Go are you using (go version)?

1.8.3

What operating system and processor architecture are you using (go env)?

linux / amd64

What did you do?

https://play.golang.org/p/A3bUPA268d

package main

import (
	"fmt"
	"time"
)

func main() {
	fmt.Println("Hello, playground", time.isLeap(2000))
}

I'm pretty new with golang, I wanted to progress and start some exercises from exercism.io!

the idea was to use the isLeap method from time pacakge, as far I understand function starting with low letter are not exported (public) and visible from other packages. Am I wrong?

In this huge repo, is there some rules to choose if a method should be exported or not?

What did you expect to see?

I'd like to avoid duplicate isLeap method as long Go SDK could provide it for me!

expected output

Hello, playground true

What did you see instead?

tmp/sandbox140448971/main.go:9: cannot refer to unexported name time.isLeap
tmp/sandbox140448971/main.go:9: undefined: time.isLeap

As I'm new here, I hope I haven't miss something evident! I guess this issue can be a bit controversial or subjective please close it if too many people speak (loose time) about it!

I through it's a good start to learn the contributing flow, so if some people are Ok with this proposal I would be please to follow the contributing Guide!

@ALTree ALTree changed the title Proposal to export time.isLeap func proposal: time: export time.isLeap Jul 7, 2017

@gopherbot gopherbot added this to the Proposal milestone Jul 7, 2017

@gopherbot gopherbot added the Proposal label Jul 7, 2017

@ALTree ALTree modified the milestones: Go1.10, Proposal Jul 7, 2017

@ALTree

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

Am I wrong?

You are correct: the function is there but it's not exported:

func isLeap(year int) bool {
	return year%4 == 0 && (year%100 != 0 || year%400 == 0)
}

I'm not sure it would be worth it to make the time API bigger (it is already quite big) for a function this simple to write.

I've tagged this as a proposal, leaving for others to decide.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

There's a cost (maintenance, documentation, cognitive load from doc/API bloat) to exporting unnecessary or uncommon things.

The proposal process and gathering the data to justify why this should be exported and showing how many people would benefit will be more work than just doing the exercism exercise.

I doubt many people would need IsLeap.

You can just copy it (as @ALTree showed above). Even if you didn't know the leap year rules, you could use the time package to extract the information too:

func IsLeap(year int) bool {
	lastDayFeb := time.Date(year, time.February, 28, 0, 0, 0, 0, time.UTC)
	nextDay := lastDayFeb.AddDate(0, 0, 1)
	return nextDay.Month() == time.February
}

(but that's less efficient than just year%4 == 0 && (year%100 != 0 || year%400 == 0))

@petrus-v

This comment has been minimized.

Copy link
Author

commented Jul 8, 2017

Thanks for replies, I get your point.

And it's true I've probably used less than 10 times isLeap method on professional projects for at least 10 years in other languages. So we are letting that cost to projects that require isLeap function, I'm closing this issue!

@petrus-v petrus-v closed this Jul 8, 2017

@golang golang locked and limited conversation to collaborators Jul 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.