Skip to content

Overhaul tool usage in CI and for contribution - #591

Merged
homersimpsons merged 11 commits into
exercism:mainfrom
mk-mxp:phpcs-config-optimisation
Feb 18, 2024
Merged

Overhaul tool usage in CI and for contribution#591
homersimpsons merged 11 commits into
exercism:mainfrom
mk-mxp:phpcs-config-optimisation

Conversation

@mk-mxp

@mk-mxpmk-mxp commented Dec 6, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor
  • Put all code styling rules into PHPCS config
    • Add XML namespace for completeness
    • Remove non-existent type="int"
    • Add directory rules for improved test coverage and required exceptions
    • Fix the new upcoming violations
  • PHPCS config renamed to default name
  • Moved tool installation to composer
  • Moved tool usage to composer
    • Allows running all unit tests now, too
  • Overhauled README

This is part of the discussion in the forum.

neenjaw
neenjaw previously requested changes Dec 6, 2023
View reviewed changes

@neenjawneenjaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Couple small changes needed. I'm fine with putting it in one place.

Comment threadexercises/concept/city-office/.meta/exemplar/Form.php
Comment threadexercises/concept/windowing-system/.meta/exemplar.php
@mk-mxp

mk-mxp commented Dec 6, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Should I do these also?

@mk-mxp
mk-mxp requested a review from neenjawDecember 6, 2023 17:29
@mk-mxpmk-mxp changed the title Put all code styling rules into PHPCS configOverhaul tool usage in CI and for contributionDec 7, 2023
@mk-mxp

mk-mxp commented Dec 7, 2023

Copy link
Copy Markdown
ContributorAuthor

@neenjaw Please take a look now. I may split this into separate PRs, also.

@mk-mxp

mk-mxp commented Dec 9, 2023

Copy link
Copy Markdown
ContributorAuthor

@neenjaw I think, GitHub is confused because I'm not assigned to this PR and so my changes do not count for review resolving.

@mk-mxp

mk-mxp commented Dec 27, 2023

Copy link
Copy Markdown
ContributorAuthor

@neenjaw The latest version of PHP CodeSniffer is not compatible to the latest Slevomat Coding Standard. I did exclude the updated PHP CodeSniffer for installation, but that is of course only temporary.

@mk-mxp

mk-mxp commented Dec 27, 2023

Copy link
Copy Markdown
ContributorAuthor

Problem:

"Invalid sniff properties set for individual sniffs will now result in an error and halt the execution of PHPCS"
PHPCodeSniffer Release notes V3.8.0

Solution:

  • Change Slevomat Sniff property to current name

This was referenced Dec 27, 2023
Closed
Closed
@neenjaw

neenjaw commented Dec 27, 2023

Copy link
Copy Markdown
Collaborator

Can you cherry pick a patch to fix ci to a separate pr to unblock the other prs?

I haven't had time to review this one as it comprises a more complex change for the track.

@mk-mxp
mk-mxpforce-pushed the phpcs-config-optimisation branch from 8d614ea to fe47754CompareDecember 28, 2023 17:30
@mk-mxp

mk-mxp commented Dec 28, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Rebased to solve merge conflicts.

Might it help to split this PR into 2 or 3? One with the rules / changes due to rules, one with the README and contribution workflow changes and maybe the removal of the old scripts also in one extra?

What I checked for workflow changes / script removal:

  • This projects CI does not use the removed scripts. It uses composer for PHP CodeSniffer and direct PHAR download for PHPUnit
  • PHP test runner does it's own thing
  • PHP representer also has nothing to do with these workflow files
  • I haven't found any hint on CI workflows other than configlet stuff in Exercism's documentation
  • We use composer during contribution workflow (locally and in CI) only, with no potential for package version conflicts because of the nature of the exercises
  • composer is the only tool used for contribution now, running the exact same things in CI and locally (if locally up-to-date, of course)

@mk-mxpmk-mxp mentioned this pull request Jan 30, 2024
Merged
6 tasks
@mk-mxpmk-mxp mentioned this pull request Feb 8, 2024
Closed
@mk-mxpmk-mxp self-assigned this Feb 16, 2024
@mk-mxpmk-mxp added x:action/improve Improve existing functionality/content x:knowledge/intermediate Quite a bit of Exercism knowledge required x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) x:size/small Small amount of work x:rep/small Small amount of reputation labels Feb 16, 2024
@mk-mxp

mk-mxp commented Feb 16, 2024

Copy link
Copy Markdown
ContributorAuthor

@ErikSchierboom Please advise on the following:

This MR changes the contribution workflow for the PHP track massively. One part is removing install.sh and install-* completely. They are not used in CI since long and now replaced by composer install for contributors.

Does this have any negative impact on you or other parts of exercism (e.g. mass MRs)?

homersimpsons
homersimpsons requested changes Feb 18, 2024
View reviewed changes

@homersimpsonshomersimpsons left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's get this merged, I did suggest some re-wording, feel free to discuss them.

Comment threadREADME.md Outdated
Comment threadREADME.md Outdated
Comment threadREADME.md Outdated
Comment threadREADME.md Outdated
Comment threadREADME.md Outdated
Comment threadREADME.md Outdated
Comment threadphpcs.xml Outdated
@mk-mxpmk-mxp mentioned this pull request Feb 18, 2024
Open
30 tasks
@mk-mxp
Put all rules into PHPCS config
2206b68
- Add XML namespace for completeness
- Remove non-existent `type="int"`
- Add directory rules for improved test coverage and required
exceptions
- Fix the new upcoming violations
@mk-mxp
Revert visibility for certain concept exercises
bf8501f
- Revert adding visibility
- Exclude those exercises from PHPCS rule
@mk-mxp
Use phpunit from composer in contribution workflow
0aaabc4
@mk-mxp
Improve style checker instructions
4cdfbc5
@mk-mxp
Rename PHPCS rule to default name and omit it
da44d36
For testing CI test file coverage, print verbose output
@mk-mxp
Run PHPCS quietly again
31efa66
@mk-mxp
Require empty line after declare(strict_types=1);
afaeda0
This is actually present everywhere in the code base, but not explicitly covered by the rules.
@mk-mxp
Point links in README to phpcs.xml, not old name
14e814a
@mk-mxp
Exclude all */.meta/*.php from strict typing
f8ee86d
The file names are set in `.meta/config.json`. And some exercises have
more than one file as example solution.
@mk-mxp
Update PHPCodeSniffer to 3.9
21f15df
@mk-mxp
mk-mxpforce-pushed the phpcs-config-optimisation branch from 507f8aa to 21f15dfCompareFebruary 18, 2024 18:06
@mk-mxp
mk-mxp removed the request for review from neenjawFebruary 18, 2024 18:23
@mk-mxp
mk-mxp dismissed neenjaw’s stale reviewFebruary 18, 2024 18:41

Reviewer has no time to follow up

@homersimpsons
homersimpsons merged commit d3a686c into exercism:mainFeb 18, 2024
@homersimpsons

homersimpsons commented Feb 18, 2024

Copy link
Copy Markdown
Contributor

Congratulations @mk-mxp let's continue !

@mk-mxp
mk-mxp deleted the phpcs-config-optimisation branch February 19, 2024 10:26
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@homersimpsonshomersimpsonshomersimpsons approved these changes
@neenjawneenjawneenjaw left review comments

Assignees

@mk-mxpmk-mxp

Labels

x:action/improveImprove existing functionality/contentx:knowledge/intermediateQuite a bit of Exercism knowledge requiredx:rep/smallSmall amount of reputationx:size/smallSmall amount of workx:type/codingWrite code that is not student-facing content (e.g. test-runners, generators, but not exercises)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mk-mxp@neenjaw@homersimpsons