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

Function builtin expanded notation #142

Merged
merged 1 commit into from Oct 8, 2018
Merged

Conversation

Mouvedia
Copy link
Collaborator

@Mouvedia Mouvedia commented Sep 6, 2018

This detail was missing from the specification.

README.md Outdated Show resolved Hide resolved
Copy link
Owner

@ericelliott ericelliott left a comment

Choose a reason for hiding this comment

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

See question.

@Mouvedia
Copy link
Collaborator Author

Mouvedia commented Oct 5, 2018

@ericelliott I don't wanna be annoying but better be safe than sorry: squash the commits.

@ericelliott
Copy link
Owner

Why?

@Mouvedia
Copy link
Collaborator Author

Mouvedia commented Oct 6, 2018

To have a cleaner history? One commit per PR is a good rule of thumb. And GitHub supports it in its interface directly.

@ericelliott
Copy link
Owner

ericelliott commented Oct 6, 2018

I prefer a more complete and accurate history. Does this actually block your workflow if it's not squashed, or is it just one of your pet peeves? =)

If it impedes your workflow, please give me an example of HOW it impedes your workflow.

@Mouvedia
Copy link
Collaborator Author

Mouvedia commented Oct 6, 2018

Here's a blog post that explains it in details: https://blog.github.com/2016-04-01-squash-your-commits/#whats-changing

On all the open sources projects that Iv contributed to, the maintainers squash the commits that result from a review. It's not my pet peeve, it's just a convention on GitHub. I mean it was quite obvious that a and String commit message shouldn't be part of the history…

On my personal projects, I go even further and respect the conventional commits specification.

@ericelliott
Copy link
Owner

Ah, so pet peeve. ;)

@Mouvedia
Copy link
Collaborator Author

Mouvedia commented Oct 6, 2018

You should not be proud of having Update README.md for almost all of your commit messages :)

Well anyway, we are not on the same page on this so Ill squash myself.

@Mouvedia
Copy link
Collaborator Author

Mouvedia commented Oct 6, 2018

@ericelliott LGTM

@ericelliott ericelliott merged commit 73a58ba into master Oct 8, 2018
@ericelliott ericelliott deleted the Mouvedia-Function branch October 8, 2018 01:03
@ericelliott
Copy link
Owner

You should not be proud of having Update README.md for almost all of your commit messages :)

We should be more descriptive about what feature we're updating. =)

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.

None yet

2 participants