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

Error when committing without "author" #107

Closed
juancampa opened this issue Mar 20, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@juancampa
Copy link
Collaborator

commented Mar 20, 2018

In the documentation author doesn't appear as required for commit but this error is thrown:

models.js:440 Uncaught (in promise) TypeError: Cannot read property '_named' of undefined
    at isNamedSection (models.js:440)
    at GitConfig.get (models.js:460)
    at config (commands.js:135)

The error goes away if I specify author.

My guess is that all commits must have an author so it should be marked as required.

@juancampa

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2018

I guess it's not required because you can also set it using config. Maybe the "fix" is to document this behavior. @wmhilton let me know what you think is best and I can post a PR

@wmhilton

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

I guess it's not required because you can also set it using config.

That's exactly it. I think the fix will actually be to throw a better error. I'm guessing it's throwing that error because it can't find a "user" section in the config. @TomasHubelbauer actually has a PR to fix a problem in the config parser, so once I merge that, I'll fix this, and then the config should just return undefined instead of throwing an error, and then I'll add a check to make sure that author.name && author.email are not undefined, and if THAT is the case, throw a nice "Author name and email must be specified as an argument or in the .git/config file" error.

@wmhilton wmhilton added the bug label Mar 21, 2018

@wmhilton wmhilton closed this in #118 Apr 1, 2018

wmhilton added a commit that referenced this issue Apr 1, 2018

@wmhilton

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

🎉 This issue has been resolved in version 0.9.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wmhilton

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

@TomasHubelbauer @juancampa In lieu of writing a CONTRIBUTING document, I slapped together a blog post: https://isomorphic-git.github.io/blog/2018/04/02/contributing-workflow.html Let me know what you think! I'll take your leftover questions and use them to write a proper CONTRIBUTING guide in the near future.

@TomasHubelbauer

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2018

@wmhilton I am on Windows and for me post-checkout hooks and fixture generator don't work. The fixture generator specifically assumes \tmp which I don't have (there). I will try to see if I can adapt that for Windows if you want to extend the support to that platform? Asking beforehand because this is a potentially non-trivial decision as supporting Windows usually means special-casing a lot of flows which would otherwise be perfectly portable on Mac and Linux. (And some projects opt to just tell people to use WSL so they don't have to support NT but WSL is only on Windows 10).

BTW the guide is really good, thank you for writing it down.

@wmhilton

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

Yeah, I've noticed that on my laptop, but not on my desktop. (I develop on Windows 10 as my primary OS.)
I think it just "happens" to work in Git Bash because it creates a virtual link from /tmp to %TEMP%.

I'll drop a PR at Thinkmill/jest-fixtures#4

@TomasHubelbauer temporarily you can try npm install https://github.com/wmhilton-contrib/jest-fixtures#win32 If that works, then we can switch the devDependency to point at the fork, or if the PR gets merged and published then we can just update the version.

edit: I ran into one more problem running the tests on my laptop, so I finally made PR #119 which should fix these issues

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