-
-
Notifications
You must be signed in to change notification settings - Fork 550
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 sync documentation #1799
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I did some nitpicking, partly unrelated to the canonical_data_syncer
to configlet sync
change - but is still in the scope of "update sync documentation"
And I've checked that, with this PR, running
$ git grep 'syncer'
in this repo produces no output.
Co-authored-by: ee7 <[email protected]>
Pinging @exercism/reviewers for a review. This is an important docs update - the current wording has probably already caused a lot of confusion. Before this PR, the README for |
@SleeplessByte Care to review? |
I'll update this PR to reflect the latest changes to |
Avoid saying "looks like this" because: - It could imply that every `tests.toml` for `two-fer` has an `include = false`. - We have omitted the comment that begins a `tests.toml` file.
Done. I've also added some extra docs. In the future, it would be nice if we didn't have to maintain 3 sets of |
Thanks! Ping @exercism/reviewers |
Minor suggestions aside... lgtm |
@hiljusti: Thanks for the review. You raise some good points about the wording, but it's a little tricky to find a great one. Some things to consider:
@ErikSchierboom any thoughts on the wording here? Also: in the future, can we have a single source of truth for |
All sounds reasonable to me |
In my experience, I've not found this to be true. Bash isn't even installed by default on Windows, so I don't think we can assume Windows users will just be able to run the Bash command.
I think having the documentation be in exercism/configlet and referring to that from the docs and prob-specs repos makes the most sense. |
I'm fine with not further specifying OSs. It's kinda minor; I think Windows developers will recognize a Rambly thoughts follow: Since this targets track maintainers, I think it's less important what the OS image comes with and more important what developers will have installed. I'm not a Windows expert, though I do have one machine that runs it, and I even ran a Windows machine for a few weeks recently for work... though I just used it as a shell to reach other remote hosts I could use for development Anyway - I know Bash won't be installed on Windows by default, but what is the developer setup these days for a Windows dev? Not all Linux distros will have |
Correct, until we centralize the documentation in one place, in which case it will become aimed at students too.
Unless you're running things exclusively in WSL, Windows users will probably have installed Git locally too. That means they'll have Git bash, but I never used in my 10+ years of running Git on Windows. |
I use git bash all the time, but it's important to note that those bash scripts only work in a git bash environment, which you're not always in. I don't use WSL. I would prefer keeping both the "cross-env-if-you're-bash-like" and "always-works-on-windows" scripts. |
Co-authored-by: J.R. "hiljusti" Hill <[email protected]>
Co-authored-by: Victor Goff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly liked @hiljusti's other suggestion, I just wanted to:
- avoid the possible reading of "the bash script cannot be run on Windows"
- clarify that the PowerShell script is Windows-only
As a final thing before we merge this - how about this?
Co-authored-by: ee7 <[email protected]>
Thanks everyone! |
The current documentation for
tests.toml
files was still referring to the obsolete canonical data syncer. This PR updates the documentation to use configlet'ssync
command instead. It also updates the contents of the exampletests.toml
file to the latest spec.Closes exercism/configlet#124