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

Remove unused imports #798

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@gabrielelana
Contributor

gabrielelana commented Dec 30, 2016

No description provided.

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Dec 30, 2016

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Dec 30, 2016

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold Dec 30, 2016

Contributor

👎 I'm pretty sure that core follows different rules for imports than other Elm programs, so these imports are actually necessary.

Contributor

mgold commented Dec 30, 2016

👎 I'm pretty sure that core follows different rules for imports than other Elm programs, so these imports are actually necessary.

@nicolaiskogheim

This comment has been minimized.

Show comment
Hide comment
@nicolaiskogheim

nicolaiskogheim Dec 31, 2016

@mgold But you're not sure?

nicolaiskogheim commented Dec 31, 2016

@mgold But you're not sure?

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold Dec 31, 2016

Contributor

No, I'm not positive.

Contributor

mgold commented Dec 31, 2016

No, I'm not positive.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 31, 2016

Contributor

Actually, @mgold is right that default import rules do not apply to the core library. At the same time, this does not matter for this PR, since the imports removed are truly unused in the respective files.

Contributor

jvoigtlaender commented Dec 31, 2016

Actually, @mgold is right that default import rules do not apply to the core library. At the same time, this does not matter for this PR, since the imports removed are truly unused in the respective files.

@jvoigtlaender jvoigtlaender reopened this Dec 31, 2016

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 7, 2018

Member

There is no harm in having extra imports. In the development build, only the top-level declarations that are needed are included, so it does not matter about imports for that. It can lengthen compile time, but the dev build also caches all packages, so they are only built once. That means there is no perf difference ever. So I think it is easier to just keep things as is. It's not hurting anything, and it'll take longer to rollback if something is off.

Member

evancz commented Mar 7, 2018

There is no harm in having extra imports. In the development build, only the top-level declarations that are needed are included, so it does not matter about imports for that. It can lengthen compile time, but the dev build also caches all packages, so they are only built once. That means there is no perf difference ever. So I think it is easier to just keep things as is. It's not hurting anything, and it'll take longer to rollback if something is off.

@evancz evancz closed this Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment