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

Avoid double definition of KeyCode alias #382

Merged
merged 1 commit into from Aug 31, 2015

Conversation

Projects
None yet
2 participants
@jvoigtlaender
Contributor

jvoigtlaender commented Aug 31, 2015

Previously existed both in Char and Keyboard modules.

Now that the next compiler version will have dead code elimination, there should be no harm in importing the alias from one into the other.

Defining KeyCode only in one place makes for more future-proofness.

(Also, the KeyCode in the type signatures in Char module will turn into links to the definition site in Keyboard module on the package site, so there's no harm in terms of documentation.)

evancz pushed a commit that referenced this pull request Aug 31, 2015

Merge pull request #382 from jvoigtlaender/keycode
Avoid double definition of KeyCode alias

@evancz evancz merged commit aa1ce6d into elm:master Aug 31, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 31, 2015

Member

Nice, this makes a lot of sense!

Member

evancz commented Aug 31, 2015

Nice, this makes a lot of sense!

@jvoigtlaender jvoigtlaender deleted the jvoigtlaender:keycode branch Aug 31, 2015

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 10, 2015

Member

This introduces a dependency cycle.

  ┌────┐
  │   Keyboard
  │    ↓
  │   Set
  │    ↓
  │   Dict
  │    ↓
  │   String
  │    ↓
  │   Char
  └────┘

I think the centralized thing needs to live in Char. Not sure yet though.

Member

evancz commented Sep 10, 2015

This introduces a dependency cycle.

  ┌────┐
  │   Keyboard
  │    ↓
  │   Set
  │    ↓
  │   Dict
  │    ↓
  │   String
  │    ↓
  │   Char
  └────┘

I think the centralized thing needs to live in Char. Not sure yet though.

evancz pushed a commit that referenced this pull request Sep 10, 2015

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 10, 2015

Member

Alright, I did the reverse of this and it is all set!

The master branches of elm-compiler / elm-package / elm-make are able to produce code again. It will not run properly, but we can at least get all the normal error reporting again.

Member

evancz commented Sep 10, 2015

Alright, I did the reverse of this and it is all set!

The master branches of elm-compiler / elm-package / elm-make are able to produce code again. It will not run properly, but we can at least get all the normal error reporting again.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Sep 10, 2015

Contributor

Ah, I had tried to check the module imports for dependencies manually, but missed that path. (Compiling to get the check automatically was not possible at the time.)

Contributor

jvoigtlaender commented Sep 10, 2015

Ah, I had tried to check the module imports for dependencies manually, but missed that path. (Compiling to get the check automatically was not possible at the time.)

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 10, 2015

Member

No worries, it only started working again in the past few days :)

Member

evancz commented Sep 10, 2015

No worries, it only started working again in the past few days :)

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