Skip to content

Improve practice exercise generator script #1648

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

Merged
merged 120 commits into from
Mar 14, 2023

Conversation

dem4ron
Copy link
Member

@dem4ron dem4ron commented Mar 9, 2023

  • Lowercase script-scoped variables to avoid collision with env vars
  • localize function-scoped variables for similar reason
  • remove function from function declarations and follow this convention everywhere so a function looks like this:
fn_name(){
...
}
  • Add .shellcheckrc - currently not seen by CI
  • figure out why
  • remove unused imports
  • setup shellcheck import paths

dem4ron and others added 30 commits February 21, 2023 17:43
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
@dem4ron dem4ron marked this pull request as ready for review March 14, 2023 10:04
@@ -87,7 +87,9 @@ jobs:
uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0

- name: Run shellcheck
uses: ludeeus/action-shellcheck@00cae500b08a931fb5698e11e79bfbd38e612a38 # v2.0.0
uses: ludeeus/action-shellcheck@master
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert back to using a commit as the version to use?


# ==================================================

# build configlet
# Build configlet
./bin/fetch-configlet
Copy link
Member

Choose a reason for hiding this comment

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

This is trying to fetch configlet a second time. I guess this is no longer needed?

Comment on lines 40 to 47
# Check if configlet is already fetched
if [ -e bin/configlet ]; then
message "success" "Configlet found!"
else
message "info" "Fetching configlet"
bin/fetch-configlet
message "success" "Fetched configlet successfully!"
fi
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 always fetch configlet, as that way you're guaranteed to work with the latest version.

@dem4ron dem4ron changed the title Stub generator patch Improve practice exercise generator script Mar 14, 2023
@dem4ron dem4ron merged commit e84875f into exercism:main Mar 14, 2023
@dem4ron dem4ron deleted the stub-generator-patch branch March 16, 2023 09:26
@petertseng
Copy link
Member

These commits should not have been 120 individual commits but instead combined into fewer commits that each contain only one operation. I think the organisation doesn't have an organisation-wide document for git commit discipline since last I checked it was track-level policy, so it is my fault for not having written a policy document for the track. Add yet another thing to the long list of things to do, then.

@dem4ron
Copy link
Member Author

dem4ron commented Mar 20, 2023

@petertseng I'm very sorry, it is all my fault. I know I should've squashed the commits, but made a mistake. I thought squash-merge was the default behaviour as it is on most of the repos and clicked the green button out of excitement, but then realised my mistake. I'm very sorry, I feel very bad for messing up the timeline.

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.

6 participants