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

Features/arrays new pony rt #81

Merged
merged 8 commits into from
Feb 9, 2015
Merged

Conversation

EliasC
Copy link
Contributor

@EliasC EliasC commented Feb 7, 2015

Tested and integrated with the new runtime

@kaeluka
Copy link
Contributor

kaeluka commented Feb 9, 2015

Awesome feature, code looks nice and it does not break any tests on my machine. (here comes the 'but'...)

BUT:

  • it lacks a test/tests
  • as this is extending user-facing syntax, there should be documentation for it
  • could you squash the commits into one so the feature is visible at a glance? (this one is less important)

@EliasC
Copy link
Contributor Author

EliasC commented Feb 9, 2015

  • There was a test, but apparently I forgot to add it.
  • Documentation is a very good point! Will add.
  • Since this will be a merge, there will be a single commit that adds the arrays. Isn't there a point to show the actual history in the branch?

@kaeluka
Copy link
Contributor

kaeluka commented Feb 9, 2015

@ 1: Awesome!
@ 2: Awesome!
@ 3: You've got a point, not sure what'd be "better" -- but do what's easy (which is leaving it as is, I suppose ^^)

@supercooldave
Copy link

@ 3: It would be useful for us newbies if you gave at least a hint of what the required commands should be.

@kaeluka
Copy link
Contributor

kaeluka commented Feb 9, 2015

@supercooldave If you have 6 commits that you want to, for instance, squash together into one big commit, you can type: git rebase -i HEAD~6. The -i stands for interactive, the argument after says "the 6 commits, starting with HEAD". Your favourite editor will open a generated file and show you 6 lines for the last 6 commits, like so:

pick c137aac Added runtime library for arrays
pick 4bfeacf Parser support for array types
pick d7fc737 Typechecking support for array types
pick 9464282 Translation of array types
pick 88a2a23 Translation of array expressions
pick 51aec36 Adapted array lib to changes in the new branch

# Rebase e968eb9..51aec36 onto e968eb9
#
# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x, exec = run command (the rest of the line) using shell
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.

You can edit those lines, save and close and git will use the file as input. There's comments in that file, explaining what to do. Elias could have replaced the last five instances of the pick keyword by squash, or just s and saved+closed and have seen another editor window with this:

# This is a combination of 6 commits.
# The first commit's message is:
Added runtime library for arrays

# This is the 2nd commit message:

Parser support for array types

# This is the 3rd commit message:

Typechecking support for array types

# This is the 4th commit message:

Translation of array types

# This is the 5th commit message:

Translation of array expressions
...

So here, git is showing the 6 commit messages that are about to become one -- you can summarise them all nicely into one message, save+close and git log will now show this:

commit d2c2afcda26cca54985708a163e89affd925b68e
Author: Elias Castegren <elias.castegren@gmail.com>
Date:   Tue Feb 3 11:55:21 2015 +0100

    Instead of the 6 commits from before, we now have one!

    Yay :)

commit e968eb971adf8bc2cb1dc1c267ee08766595c34d
Author: Tobias Wrigstad <tobias.wrigstad@it.uu.se>
Date:   Sat Feb 7 00:35:43 2015 +0100

    Backported trace function for futures

So rebase -i is a wonderful feature for documenting what you've been doing better.

HOWEVER, it's generally not save to merge commits that you have already pushed to a place where others may have pulled them. In this case, the pull request, it would've been fine (right, @kikofernandez ?) since we talked about it and I knew what was going to happen.

@supercooldave
Copy link

This is great. Thanks.

@kaeluka
Copy link
Contributor

kaeluka commented Feb 9, 2015

:) I agree!

@kaeluka
Copy link
Contributor

kaeluka commented Feb 9, 2015

Thanks! Btw, the cardinality syntax |x| for the array length is nice!

kaeluka pushed a commit that referenced this pull request Feb 9, 2015
@kaeluka kaeluka merged commit 1c993df into new-ponyrt Feb 9, 2015
@kaeluka kaeluka deleted the features/arrays-new-pony-rt branch February 9, 2015 10:51
@EliasC
Copy link
Contributor Author

EliasC commented Feb 9, 2015

@kaeluka In the future, one could consider letting |x| be syntactic sugar for x.size()

@TheGrandmother
Copy link
Contributor

|x| would be incredibly neat!

@kaeluka
Copy link
Contributor

kaeluka commented Feb 9, 2015

@TheGrandmother it is incredibly neat. That's what's implemented now.

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