- First commit to issue fork.
- 🇫🇷France dydave
Since the module is now integrated with Gitlab CI, let's try bringing back this issue at the top of the stack:
See initial CSPELL error report:
https://git.drupalcode.org/issue/token-3157138/-/jobs/1681579
(where we started)
Quick follow-up on this ticket:
Created initial merge request MR!76 based on the patch provided at #9, which didn't apply completely anymore, but still managed fixing almost all the errors that were reported by the CSpell job on Gitlab CI.
A few initial commits were added to the current merge request MR!76 with a few technical details, see above at #12.
But mostly, at this point:
Last build on MR!76: https://git.drupalcode.org/issue/token-3157138/-/pipelines/181574- The CSPELL job now seems to be passing ✅
CSPELL job: https://git.drupalcode.org/issue/token-3157138/-/jobs/1683065
- The PHPUnit Tests are passing as well 🟢 \o/
PHPUnit job: https://git.drupalcode.org/issue/token-3157138/-/jobs/1683066
These changes are harmless enough, without any change in module's code (only comments) so they shouldn't take too long to review and would help enforcing automated spell check validation for other pending merge requests.
We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!76 and let us know if there would be any more work needed.
Feel free to let us know if you have any questions or concerns on merge request MR!76 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews. - The CSPELL job now seems to be passing ✅
- First commit to issue fork.
- 🇫🇷France dydave
@ankitv18:
Why did you change that Ankit?
https://git.drupalcode.org/project/token/-/blob/43a454040745db69b227ccf8...Do you think it's better to display
cspell disable
in the README file?Moving maintainers to a specific file is a "legit" change and the file is explicitly excluded from CSpell validation on GitlabCI...
Personally, I don't think showing cspell disable on the README page would be acceptable...
I would be in favor of reverting this change:
https://git.drupalcode.org/project/token/-/merge_requests/76/diffs?commi...Not sure if the other change is really needed, but if it is then fine.
- 🇮🇳India ankitv18
Following up the https://project.pages.drupalcode.org/gitlab_templates/jobs/cspell/ I've implemented as per the suggestion and
bifurcating the maintainers part from the README.md is doable? then that is something I will implement the same.
In many cases I've seen maintainer added the cspell_words with the maintainers name in the gitlab-ci.yml and some maintainer keep cspell:disable, so it is bit confusing how to handle this part. - 🇫🇷France dydave
It's not really helping @ankitv18 ...
Aren't there enough tickets, bugs and issues going around where you could really be fixing something instead of trying to mess up with a ticket which was fine and didn't really need help anymore?
There are plenty of helpful ways you could be earning credits if that's what you're looking for, but personally I'm really not convinced any of the changes you've done to this MR add any value to this ticket...
- 🇫🇷France dydave
Hi Ankit (@ankitv18),
I cloned your changes to a separate branch:
https://git.drupalcode.org/issue/token-3157138/-/tree/3157138-gitlabci-f...Rolled back MR!76 and the CSpell tests seem to be passing again now ✅
see pipeline:
https://git.drupalcode.org/issue/token-3157138/-/pipelines/222961Let's hope a maintainer could help us get these changes merged in first, since all CSpell errors are fixed and the job completes successfully.
Then perhaps we could consider making other changes to the project in other tickets.
Thanks! - 🇺🇸United States tr Cascadia
Actually, I said D7, but this is an issue for the 8.x-1.x branch so I don't know what I was thinking ... maybe just seeing the t() function triggered me ...
Regardless, I think the argument still holds, although it is less strong in the case of the 8.x-1.x branch. We probably shouldn't be changing translatable strings in patch releases.
- 🇮🇳India ankitv18
Hi @Dydave,
First of all I really appreciate of your doing work and learnt quite lot of things.
Contribution and community is all about learning and improving new things. I already mentioned the reason of my commit.
And if you want I can share the commit of yours where you have just put REAMD.md in cspell_ignore_path variable.
So I'm not trying to earn credits and all. I'm here to learn while contributing and helping out.So in one of my commit I have added one project words i.e mlid rest of changes can be irrelevant.
- 🇫🇷France dydave
Thanks a lot Tim (@TR) for the super helpful feedback and review above at #19.
I didn't really take into consideration BC and potential issues with translated string keys... I'll pay much more attention in future merge requests for other potential projects.
The corresponding change was reverted and it seems the word
e-mail
has been added to Core's allowed list.
In other words, reverting the line didn't prompt an error anymore, so no problem getting this fixed.It seems the job is still passing all phpunit tests, see the latest pipeline in MR:
https://git.drupalcode.org/issue/token-3157138/-/pipelines/225798With the CSpell job completing successfully:
https://git.drupalcode.org/issue/token-3157138/-/jobs/2134518Feel free to let us know if you have any more comments, reviews and or feedback, we would certainly greatly appreciate your help!
Thanks in advance! - Status changed to Fixed
9 months ago 10:12pm 28 July 2024 - 🇨🇭Switzerland berdir Switzerland
Merged. Various good typo fixes here, a bit unsure on making cspell not allowed to fail, but unlike coding standards, which might change, it seems unlikely to change on us on the future.
As often, forgot to check credits section before merging, but it's adjusted now, so only the commit message is wrong.
- 🇫🇷France dydave
Thanks for taking the time to look at this Sascha (@Berdir), for accepting the changes and the credit on the issue, it's greatly appreciated!
Automatically closed - issue fixed for 2 weeks with no activity.