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

First pass at Main README.md - Issue #278 #334

Merged
merged 17 commits into from
Sep 20, 2017

Conversation

sacherjj
Copy link
Contributor

Two places with !!!!!

README.md for each exercise is currently checked in. Should this not happen? I know i just did amends on a bunch, because I didn't have the correct source (even though it is generated later).

Last section, I'm not sure what goes on with /bin. Easiest solution it to not include this at first.

@sacherjj sacherjj changed the title First pass at Main README.md #287 First pass at Main README.md - Issue #287 Jul 25, 2017
@sacherjj sacherjj changed the title First pass at Main README.md - Issue #287 First pass at Main README.md - Issue #278 Jul 25, 2017
Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

thanks. I will probably have more things to say later since I am short on time and can't give this the thorough time it deserves. But it seems you had important questions and so it is good to give an answer to them.

README.md Outdated

- Be sure to add it to the appropriate place in the `config.json` file. Position in the file determines the order exercises are sent. Generate a unique UUID for the exercise. Current difficuly levels in use are 1, 4, 7 and 10.

- !!!!! Not sure if this is accurate !!!!! Also, please run `bin/fetch-configlet && bin/configlet` to ensure the exercise is configured correctly.
Copy link
Member

Choose a reason for hiding this comment

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

Eh... this sentence is accurate in that it is possible to do so. I'm fine with calling this an optional step, since Travis CI will do it anyway.

README.md Outdated

- Exercises should use only the Rust core libraries. If you must use an external crate, see note below about `Cargo-example.toml`. `Cargo.toml` should not have external dependencies as we don't want to make the user assume which crates are required.

- !!!!! Not sure if this is accurate !!!!! 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
Copy link
Member

Choose a reason for hiding this comment

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

you are correct. this is inaccurate. It is possible that exercism/meta#15 will lead to sufficient information to know what to replace it with.

On the other hand, you could also argue that it is not your job to fix this sentence, since it is derived from https://github.com/exercism/request-new-language-track/blob/master/TRACK_README.md . You have first dibs on filing an issue on https://github.com/exercism/request-new-language-track to ask for guidance on replacing this sentence.

If it is necessary, I would accept simply removing this entire bullet point until a proper replacement is determined.

README.md Outdated
---

- An exercise may contain `.meta/hints.md`. This is optional and will appear after the normal exercise
instructions if present. Rust is different in may ways from other languages. This is a place where
Copy link
Member

@petertseng petertseng Jul 25, 2017

Choose a reason for hiding this comment

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

may -> many

@sacherjj
Copy link
Contributor Author

Updated addressing those points.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I think this will be good. We should put this above the "Rust icon" since it is more important than the icon.

We can delete the line that says "Please see the [contributing guide](https://github.com/exercism/docs/tree/master/contributing-to-language-tracks)" since there is an identical link in the section being added.

README.md Outdated

## Contributing a New Exercise ##

- All Exercism exercises must be defined in [problem-specifications](https://github.com/exercism/problem-specifications/tree/master/exercises) before they are implemented for a specific track. Please submit a PR there if your exercise is new to Exercism.
Copy link
Member

@petertseng petertseng Jul 28, 2017

Choose a reason for hiding this comment

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

this sentence is no longer true, but that is also not your fault. exercism/generic-track#35

So, I'm going to suggest we write here If a new exercise would potentially be suitable for a track other than the Rust track, it should be defined in [problem-specifications](https://github.com/exercism/problem-specifications/tree/master/exercises) so that other tracks may benefit from it. Or something like that?

README.md Outdated

- Watch out for trailing spaces, extra blank lines, and spaces in blank lines.

- All the tests for Rust exercises can be run from the top level of the repo with `_test\check-exercises.sh` Please run this command before submitting your PR. See [Windows Specific instructions](_test/WINDOWS_README.md) for running this.
Copy link
Member

Choose a reason for hiding this comment

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

I would say / instead of \ in _test\check-exercises.sh, for consistency with other instances of directory separator in this file (exercism/discussions, exercises/exercise/name, etc.)

README.md Outdated

- All Exercism exercises must be defined in [problem-specifications](https://github.com/exercism/problem-specifications/tree/master/exercises) before they are implemented for a specific track. Please submit a PR there if your exercise is new to Exercism.

- Please make sure the new exercise conforms to specifications in the [exercism/problem-specifications](https://github.com/exercism/problem-specifications) repo. (Note if slight changes are needed for Rust
Copy link
Member

Choose a reason for hiding this comment

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

I would say / instead of \ in .meta\hints.md, for consistency with other instances of directory separator in this file (exercism/discussions, exercises/exercise/name, etc.)

README.md Outdated
the differences required for Rust are explained. If it a large change, you may want to call this out
as a comment at the top of `src/lib.rs`, so the user recognises to read this section before starting.

- If an `example.rs` uses external crates, include `Cargo-example.toml` so that `_tests\check-exercises.sh` can compile with these when testing.
Copy link
Member

Choose a reason for hiding this comment

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

I would say / instead of \ in _test\check-exercises.sh, for consistency with other instances of directory separator in this file (exercism/discussions, exercises/exercise/name, etc.)

README.md Outdated

- An exercise may contain `.meta/hints.md`. This is optional and will appear after the normal exercise
instructions if present. Rust is different in many ways from other languages. This is a place where
the differences required for Rust are explained. If it a large change, you may want to call this out
Copy link
Member

Choose a reason for hiding this comment

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

"If it a large change" - missing word

README.md Outdated
- We are currently committing Cargo.lock. This may change.

- `README.md` may be regenerated from exercism data. The top section above `## Rust Installation` should contain `description.md` from the exercise directory in the
[problem-specifications github.](https://github.com/exercism/problem-specifications/tree/master/exercises) The `## Source` section comes from the `metadata.yml` in the same directory.
Copy link
Member

Choose a reason for hiding this comment

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

I would say "problem-specifications repository" instead of "problem-specifications github" (since it may not always be clear what github means when used in that context)

README.md Outdated

- If an `example.rs` uses external crates, include `Cargo-example.toml` so that `_tests\check-exercises.sh` can compile with these when testing.

- We are currently committing Cargo.lock. This may change.
Copy link
Member

Choose a reason for hiding this comment

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

I am not convinced "This may change" is necessary, since ostensibly anything in this document could be subject to change. I would remove it

README.md Outdated

- Be sure to add the exercise to an appropriate place in the `config.json` file. The position in the file determines the order exercises are sent. Generate a unique UUID for the exercise. Current difficuly levels in use are 1, 4, 7 and 10.

- The Rust repository uses [Travis CI](https://travis-ci.org/exercism/rust/) to validate the build and automatically runs `bin/fetch-configlet && bin/configlet` to ensure the exercises are configured correctly.
Copy link
Member

Choose a reason for hiding this comment

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

Were I reading this sentence, I would ask the question "How does this affect me? Why did you tell me this?". Shall we put an answer to that after?

This also seems oddly placed since _test/check-exercises.sh is mentioned far above, and it kind of seems like they are related (both are run by Travis CI). Does it make sense to mention the two things closer together?

@sacherjj
Copy link
Contributor Author

Addressed all comments.

Added section for version number in Cargo.toml based on canonical-data.json being present. Based on changes in one of my submitted exercises.

Moved Rust icon to bottom of the file as second change.

@petertseng
Copy link
Member

An interesting thing happened since this PR was submitted: exercism/generic-track#37 was made in the request-new-language-track. Are any of those changes suitable to be included in this PR as well?

@petertseng
Copy link
Member

well OK, in that case I'll just do whatever I think is right.

@petertseng petertseng merged commit 9af9dc3 into exercism:master Sep 20, 2017
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.

2 participants