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

Datehumanizertests and implementation #1

Closed
wants to merge 4 commits into from
Closed

Datehumanizertests and implementation #1

wants to merge 4 commits into from

Conversation

chrissie1
Copy link
Collaborator

Same tests as in the .net version apart from one extra one. Might be a slightly different implementation no localization yet, all just English.

Not sure why intellij changed the idea files so you should probably not merge those. But what do I know.

Happy building.

@@ -8,7 +8,7 @@
<option name="gradleHome" value="$USER_HOME$/tools/gradle" />
<option name="modules">
<set>
<option value="$PROJECT_DIR$" />
<option value="E:\Users\christiaan\IdeaProjects\Lessthandot\Humanizer.jvm" />
Copy link
Owner

Choose a reason for hiding this comment

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

Should we revert this back?

@hhariri
Copy link
Collaborator

hhariri commented Apr 21, 2014

Yes. Not sure how those changes got int here. Also, I'm going to refactor the tests a bit to extract some function and make it less code. But awesome stuff from @chrissie1

@@ -1,11 +0,0 @@
<component name="libraryTable">
Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm, do we not need this? I have no idea :p

P.S. This is the most clueless code review I've ever done!!

@MehdiK
Copy link
Owner

MehdiK commented Apr 21, 2014

Thanks for the great contribution @chrissie1.

I honestly have no idea about Kotlin or the JVM. I left some comments on things I thought might require change! Let me know what you think.

@MehdiK
Copy link
Owner

MehdiK commented Apr 21, 2014

WRT localisation, Humanizer didn't have ANY for a year and only recently we started adding them and the required support in. So no worries for now.

@MehdiK
Copy link
Owner

MehdiK commented Apr 21, 2014

Could you also please add your cool new addition to readme file so new arrivals know what's there?

We have a release-notes file with PR entries in Humanizer. How do you guys feel about creating a release-notes file and adding PRs in?

@chrissie1
Copy link
Collaborator Author

Ok, I will add to the readme.

I cloned and then opened the project in intellij. I'm sure it asked me a question to which I said yes. I moved the project since then so the path it has there isn't even correct.
Not sure intellij will let me change them.

@chrissie1
Copy link
Collaborator Author

And I would welcome the refactor. It works but probaly not the best kotlin code out there.

red, green, refactor. I'm at the green stage ;-).

@chrissie1
Copy link
Collaborator Author

now tests for 59 minutes.

@hhariri
Copy link
Collaborator

hhariri commented Apr 22, 2014

I'm not sure what happened with project files so for now I've just copied your work and will push it. Can you re-import?

@hhariri hhariri closed this Apr 22, 2014
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

3 participants