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

Rspec fixes #3

Merged
merged 74 commits into from
Aug 10, 2023
Merged

Rspec fixes #3

merged 74 commits into from
Aug 10, 2023

Conversation

Chadowo
Copy link
Collaborator

@Chadowo Chadowo commented Aug 1, 2023

Hey there, this PR will contain all the rewrite to the RSpec tests of the project.

Details

The changes are pretty straightforward, firstly I ran Rubocop on all the tests and corrected offenses that are safe to do so, then comes updating each test to match with the modern RSpec usage,

and remove use of "should" in test descriptors
and remove use of "should" in test descriptors
and remove use of "should" in test descriptors
and remove use of "should" in test descriptors
@jlnr
Copy link
Member

jlnr commented Aug 7, 2023

That's a button I hadn't seen before, but I clicked it and it seems to work :)

Bildschirmfoto 2023-08-08 um 01 15 01

.github/workflows/rspec.yml Outdated Show resolved Hide resolved
Chadowo and others added 2 commits August 7, 2023 20:41
Co-authored-by: Julian Raschke <julian@raschke.de>
@Chadowo
Copy link
Collaborator Author

Chadowo commented Aug 7, 2023

Oh and I've just finished the new README (79bc0ca), it is mostly like the old one but with spell checks, modern Markdown syntax and some changes here and there to understand better the purpose of the text, it looks good in my opinion however I still didn't check it thoroughly.

I also wanted to get some feedback on the changes I've made, and maybe the direction I should head to if you will :D

Copy link
Member

@jlnr jlnr left a comment

Choose a reason for hiding this comment

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

The direction of the README looks great to me. 👍 I'm not sure why the GitHub Actions disappeared again. Ah well, we can look at them again in follow-up if necessary.

@jlnr
Copy link
Member

jlnr commented Aug 8, 2023

Do you have any preferences for how to merge the PR (after shuffling the README files around)? Using a merge commit will add 60+ commits to the history. I'd lean towards squashing it into a single commit for the sake of auditing "what has happened since the fork" using the Git history. Do you think that squashing loses to much information? It looks like Chingu previously used merge commits with full PR history instead.

Also, ping @ippa (hope this ping works) in case you want to give any opinions about this PR. After all, it's the first after the fork. Maybe you decide you want this to happen on the "old" repository after all :)

@Chadowo
Copy link
Collaborator Author

Chadowo commented Aug 8, 2023

The direction of the README looks great to me. +1 I'm not sure why the GitHub Actions disappeared again. Ah well, we can look at them again in follow-up if necessary.

Thanks for the feedback! and about the Github Actions, on my fork it says there's a syntax error on line 24, this is the code:

    - name: Install dependencies (Ubuntu)
        if: startsWith(matrix.platform, 'ubuntu-') # <- Here
        run: sudo apt-get update && sudo apt-get install -y libsdl2-dev libgl1-mesa-dev libfontconfig1-dev xvfb

      - name: Install dependencies (macOS)
        if: startsWith(matrix.platform, 'macos-')
        run: brew install sdl2
    # [...]

I'm not 100% sure what may it be (I'm a dummy with YML), however looking at the code around it and how other wrote their workflow files I'd suggest something like this to fix it :

    # Starting from the steps
    steps:
    - uses: actions/checkout@v3
    
    # Align if and run indentation with name
    - name: Install dependencies (Ubuntu)
      if: startsWith(matrix.platform, 'ubuntu-')
      run: sudo apt-get update && sudo apt-get install -y libsdl2-dev libgl1-mesa-dev libfontconfig1-dev xvfb

    # Align name with the one above
    - name: Install dependencies (macOS)
      if: startsWith(matrix.platform, 'macos-')
      run: brew install sdl2
  
    # [...]

I don't want to get ahead of myself with conclusions, so I'll wait to get your opinion on the matter first.

Do you have any preferences for how to merge the PR (after shuffling the README files around)? Using a merge commit will add 60+ commits to the history. I'd lean towards squashing it into a single commit for the sake of auditing "what has happened since the fork" using the Git history. Do you think that squashing loses to much information? It looks like Chingu previously used merge commits with full PR history instead.

Hmm, I would also lean towards the squashing path, in my opinion its more minimal and elegant, we could write all the info in the commit summary, it shouldn't be that much confusing in my opinion (if we properly convey the info), what do you think?

@jlnr
Copy link
Member

jlnr commented Aug 9, 2023

Regarding actions: Yes, oops, looks like I messed up the indentation in my change suggestion. This is the file I have copied it from.

Squashing: Then let's do that once the Actions work again 👍

@Chadowo
Copy link
Collaborator Author

Chadowo commented Aug 9, 2023

Ok, I'll get to it!

@Chadowo
Copy link
Collaborator Author

Chadowo commented Aug 10, 2023

I think everything's done, I've finished revising the new README, adjusted the workflow file, and organized the place. Feel free to give it an eye :D

Copy link
Member

@jlnr jlnr left a comment

Choose a reason for hiding this comment

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

One last thing, sorry about that.

chingu.gemspec Outdated Show resolved Hide resolved
jlnr
jlnr previously approved these changes Aug 10, 2023
Copy link
Member

@jlnr jlnr left a comment

Choose a reason for hiding this comment

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

I was able to commit (write) my own suggestion! Great, now I can immediately approve.

Thanks a lot for all the hard work here! Will squash & merge as soon as the tests has finished.

@jlnr jlnr enabled auto-merge (squash) August 10, 2023 09:05
Gemfile.lock Outdated Show resolved Hide resolved
.github/workflows/rspec.yml Outdated Show resolved Hide resolved
Copy link
Member

@jlnr jlnr left a comment

Choose a reason for hiding this comment

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

🥳

@jlnr jlnr merged commit 6d84966 into gosu:master Aug 10, 2023
This was referenced Aug 10, 2023
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