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

Add -Wall to all package.yaml #773

Merged
merged 3 commits into from
Nov 22, 2018
Merged

Add -Wall to all package.yaml #773

merged 3 commits into from
Nov 22, 2018

Conversation

sshine
Copy link
Contributor

@sshine sshine commented Nov 16, 2018

I added ghc-options: -Wall below source-dirs: src

library:
  ...
  source-dirs: src
  ghc-options: -Wall

for both example solutions and exercise stubs, using the following command:

find . -name package.yaml -exec perl -i -pe 's/(source-dirs: src)/$1\n  ghc-options: -Wall/s' {} \;

This causes warnings to show on both stack build and stack test.

I realize that version numbers should be bumped, too. Oh dear.

@sshine
Copy link
Contributor Author

sshine commented Nov 16, 2018

Version numbers bumped:

for p in $(git diff-tree --no-commit-id --name-only -r HEAD); do
  perl -i -pe 's/(?<=version: )(.*?)(\d+)$/"$1".($2+1)/e' $p
done

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.

were any missed?

$ find exercises -name package.yaml | xargs grep -L 'this search works' | wc -l                               (11-17 04:20)
     201
$ find exercises -name package.yaml | xargs grep -L 'ghc-options: -Wall' | wc -l                              (11-17 04:20)
       0

did any file somehow get two?

$ for i in $(find exercises -name package.yaml); do n=$(grep 'ghc-options: -Wall' $i | wc -l); if [ "$n" -ne 1 ]; then echo "no" ; fi; done

cool. be careful with two certain versions.

exercises/binary/package.yaml Show resolved Hide resolved
exercises/zipper/package.yaml Outdated Show resolved Hide resolved
In #522 the following convention was established:

> [exercises based on a non-versioned canonical-data.json should use
> version 0.9.0.1. If there is a comment containing the date from the data
> file used as a reference, it should be added to the YAML file, to ease
> transition, like this:

The 'binary' exercise appears to be the only package left that honors
this convention of non-versioned canonical data. Since the exercise is
deprecated, and canonical data has versions in them, I propose that we
deprecate this convention in favor of 'version' lines that contain only
the MAJOR.MINOR.PATCH.SERIAL version number.
@petertseng
Copy link
Member

Oh, something I just realised - The motivations written for this PR indicate that this is geared toward students.

In that case, you might consider simply not adding the line to examples' package.yaml, since they're already being run with -Wall -Werror due to https://github.com/exercism/haskell/blob/master/bin/test-example#L53 .

Then again, it's already been done, so shrug.

@sshine
Copy link
Contributor Author

sshine commented Nov 19, 2018

Good point.

But should students also have -Werror set?

@petertseng
Copy link
Member

We can probably base that decision on factors such as:

  • how strongly mentors feel about students not having warnings
  • how much additional burden it will place on students

I caution that it may cause the process of working on an exercise to be more cumbersome. I imagine that during the course of that work there are a few unused bindings, etc. Sometimes I just want to see whether it compiles, even though I have some unused bindings.

That was of course quite speculative. It may be valuable to try to complete an exercise from scratch with -Wall -Werror all the way and see what happens.

Also, bump version number.

This fixes #769.
@petertseng
Copy link
Member

Righto righto, let me do the README business and this will be good, there will be one PR and one issue for the README business

@sshine
Copy link
Contributor Author

sshine commented Nov 22, 2018

What's the README business? Ah! Should have fixed that now.

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.

OK, so now that only the non-example package.yaml are updated, the checks for this got easier, since verification operations need to act on exercises/*/package.yaml instead of find exercises -name package.yaml

$ ls */package.yaml | wc -l                          (11-22 10:47)
96
$ grep -L 'this search works' exercises/*/package.yaml | wc -l
96
$ grep -L '^  ghc-options: -Wall' exercises/*/package.yaml | wc -l 
0

So, we should be good once it's rebased.

@sshine sshine merged commit 1b73eaa into exercism:master Nov 22, 2018
@petertseng
Copy link
Member

I'd like to try really hard never to merge a Merge branch 'master' into (other branch) into master. I really hate having the unnecessary commit in the history.

In PRs that should be merged as one commit, that means Squash and Merge is suitable.

But what to do about PRs that should be merged as multiple commits, such as this one?
We found that Rebase and Merge does eliminate the Merge branch 'master' into (other branch) commit (#444 (comment)), but the disadvantage of that one is that I couldn't find a way to tell, from the command line, which PR the commits came from; that info only shows up in the UI. So I think the way to do it that meets the desires is to rebase from the command line, then use Create a Merge Commit.

I guess the other solution is to ask for the up-to-date requirement to be turned off. Honestly I thought that exercism/discussions#174 indicates there's a desire to have it on for all repos, but I know of at least two track repos that do not have that option.

@sshine
Copy link
Contributor Author

sshine commented Nov 23, 2018

I think I pushed a button that caused this extra merge from master. I really wanted to push it. But it seems like if I had rebased before pushing to my own branch, then I could have gone without clicking it.

How about if we ask committers to rebase from master as the last thing they do, since there is no "rebase from master" button, but only a "merge from master" button?

@sshine sshine deleted the Wall branch November 23, 2018 07:15
@petertseng
Copy link
Member

Yeah, that sounds right.

@sshine
Copy link
Contributor Author

sshine commented Nov 23, 2018

Excellent. Learning the GitHub workflow here. :-)

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