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

Update README with new badge and a revised contributing section #58

Merged
merged 6 commits into from
Oct 15, 2017

Conversation

Stargator
Copy link
Contributor

exercism/dart now has a Chat Room on Gitter

I just created a chat room. You can visit it here: https://gitter.im/exercism/dart.

This pull-request adds this badge to your README.md:

Gitter

If my aim is a little off, please let me know.

I figured a gitter room would be easier than a discussing in random pull requests or issues.

PS: Click here if you would prefer not to receive automatic pull-requests from Gitter in future.

@Stargator Stargator added this to the LaunchPad milestone Oct 8, 2017
@Stargator Stargator self-assigned this Oct 8, 2017
@devkabiir
Copy link
Contributor

@Stargator, I think, since we're already on Readme.md why not add a 'contributing' section, maybe a 'setup/requirements' too. And make this pull request about updating the Readme.md rather than just adding a gitter badge (it's perfectly aligned 👌), rename the PR too.

Copy link
Contributor

@devkabiir devkabiir left a comment

Choose a reason for hiding this comment

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

I'm approving because if you decide to create a separate PR for the Readme.md itself. See my comment.

@Stargator
Copy link
Contributor Author

@kabiir We have a contributing section, which does need some work. So I'm including that.

Let me know if you think anything is missing. @fwip the same goes to you, if you so desire.

@Stargator Stargator changed the title Add Gitter Badge to README Update README with new badge and a revised contributing section Oct 8, 2017
README.md Outdated
@@ -55,7 +55,7 @@ Please keep the following in mind:

- All the tests for Dart exercises can be run from the top level of the repo with ... Please run this command before submitting your PR.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding the command for running all tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha

README.md Outdated
@@ -35,7 +25,17 @@ Test files should use the following format:
# include the body of an example test
Copy link
Contributor

Choose a reason for hiding this comment

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

Either we need to come up with a generalized example or perhaps use the hello-world exercise test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, the standard practice is to use the Hello World

@Stargator
Copy link
Contributor Author

Recommend adding details about the updates to "create-exercise". I'll try and get that this weekend.

test("says hello world with no name", () {
final String result = helloWorld.hello();
expect(result, equals("Hello, World!"));
}, skip: false);

Choose a reason for hiding this comment

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

What's the purpose of this skip: false? The default behavior is not to skip a test, so this is redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. My thinking is that skip: false made it more clear to a user how to enable the other tests (change true to false). But deleting , skip: false is also possibly obvious.

How do other tracks handle this? Is there a consensus or established wisdom?

Copy link
Contributor

Choose a reason for hiding this comment

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

The automated testing switches this flag to true and then back to false after testing. Although I don't understand its purpose either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's so users focus on one failing test versus being told all the tests are failing.

Users are meant to solve the exercise one test at a time.

Now it's true, the skip: false doesn't need to be there, if that's what people were confused about.

Copy link

@rafaelalvessa rafaelalvessa Oct 12, 2017

Choose a reason for hiding this comment

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

@Stargator Ah, I see what you mean. Initially, users will skip all tests but the first one, and work their way through each test individually, to get them used to TDD. Alright, that sounds good to me.

README.md Outdated
* `bin/configlet lint .` - Checks the config.json for formatting issues
* `bin/configlet fmt .` - Formats the config.json
* `bin/check_formatting` - Checks All the Dart files for formatting issues
* `pub run dart_style:format -l 120 -w .` - Formats all the Dart files

Choose a reason for hiding this comment

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

All these commands could potentially be put into a script to save some time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a ticket to address that: #61

README.md Outdated
@@ -68,9 +76,11 @@ Please keep the following in mind:
- Please do not add a README or README.md file to the exercise directory. The READMEs are constructed using shared metadata, which lives in the
[exercism/problem-specifications](https://github.com/exercism/problem-specifications) repository. Further explanation can be found in [fixing-exercise-readmes](https://github.com/exercism/docs/blob/master/language-tracks/exercises/anatomy/readmes.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this section is out of date - we should add README.md now, if I'm understanding correctly.

See: exercism/meta#15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I read this as the "exercises" directory shouldn't have a README.

But each exercise should have a README like raindrops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so it should be suggested to generate a README file using configlet. There's a issue created to update create-exercise to do this as well.

I'll update the README to make it clear the READMEs should be generated.

@Stargator
Copy link
Contributor Author

@kabiir and @fwip Ready for you to re-review :)

@Stargator
Copy link
Contributor Author

Given our agreement on using Rebase, reminder for me to update the README to include that Rebasing is our merge strategy for Pull Requests.

Given that, it would be prudent of us to also review the commit messages themselves to ensure they don't contain anything offensive or sensitive.

@Stargator
Copy link
Contributor Author

Will merge once build is complete

@Stargator Stargator merged commit d40b1bf into exercism:master Oct 15, 2017
@Stargator Stargator deleted the gitter branch October 15, 2017 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants