Problem/Motivation

The 'help improve this page' block is not correct for coding standards.
Have to monitor changes to pages that did not go through the committee.
Difficult to find issue associated with a change..

Steps to reproduce

Proposed resolution

Convert coding standards docs to GitLab pages.

Remaining tasks

  1. Convert current docs
  2. Initial review the new markdown files
  3. Generate pages from docs</li>

These will be done in followups

  • Update links to pages
  • Check theme, menus etc
  • Create followup to add use gitlab ci, spell checking etc
  • Final check that the markdown files are up to date with what is on d.o
  • Update coding standard process to include docs MR in some step.
  • Update d.o. doc pages to redirect (or link) to new location.
  • Turn off editing of existing pages (or add a notice at top of each asking not to edit as the changes will be lost)

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#4 documentation.patch305.14 KBborisson_
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

quietone created an issue. See original summary.

quietone’s picture

Issue summary: View changes

borisson_ made their first commit to this issue’s fork.

borisson_’s picture

StatusFileSize
new305.14 KB

I can't figure out how to push to this repo. But I have a patch attached.

dww made their first commit to this issue’s fork.

dww’s picture

Status: Active » Needs review

I created the initial main branch for this repo. Then I created an MR from the issue fork.

quietone’s picture

@borisson_, does that cover everything at https://www.drupal.org/docs/develop/standards ? And probably a good idea to upload any script used?

dww’s picture

Mentioned in Slack, but I'll mention here, too...

Do we want to open a related issue about setting up a CI pipeline to run cspell on everything in this repo? Any other lint jobs we can think of?

quietone’s picture

Status: Needs review » Needs work
borisson_’s picture

#4: I used an online html to markdown provider. I did the rest by hand, so there's no script to upload.

jonathan1055’s picture

This is a great idea. Would you like some help in getting a pipeline running, to actually build the documentation site?

[edit - I did not see that this had already been mentioned in #9 above]

quietone’s picture

Status: Needs work » Needs review

Ready for review. Hopefully, this is good enough and further improvements can move to followups.

jonathan1055’s picture

This is a great start. Some initial observations

The composer job shows that the project does not have a .info.yml file and whilst this is not a blocker it means that the assumption is that the project type is "module". Maybe it would be worth adding a coding_standards.info.yml and setting the appropriate values. Other project types are 'theme' and 'distribution' but neither of them are necessarily more correct than 'module'.

There are some spelling errors.

CSpell: Files checked: 36, Issues found: 105 in 17 files.
The number of distinct unrecognised/misspelled words is 51

Some of these are probably actual spelling mistakes but many are made-up words. It would be very important eventually to have the cspell job run green so that we can instantly see any new mistakes. I think we need to go through the list of unknown words (which is avaiable as an artifact file for download) and some of the instamces could be changed to use real words. Then the remainder can go into a custom project dictionary.

I notice that the actual mkdocs jobs is not run. This is actually by design in gitlab_templates, because issue forks do not have their own documentation url, so it would be re-building the live doc site on every MR pipeline. However, in this case I think its worrth building the initial site during these pipelines, to check menus and liks etc I can cusomise the mkdocs rules temporarily for that, if you like?

quietone’s picture

Issue summary: View changes

@jonathan1055, thanks for offering to work on that. I think all that should be in a follow up. Let's keep this in scope to what is in the issue summary.

jonathan1055’s picture

Status: Needs review » Reviewed & tested by the community

OK, yes that's fine :-)
If this issue is just to take the existing pages as-is (which borisson_ says has been done) then you might as well merge this straight away. Then we'll be able to see the generated pages, test links and formatting and create follow-ups for the fixes. Gitlab Templates also has a script to check for incorrectly formetted urls in .md pages. That is only used internally in GT, but it may be useful to use that here too.

jonathan1055’s picture

Status: Reviewed & tested by the community » Needs work

Just seen there are review comments in the MR, so setting back to Needs Works.

quietone’s picture

Status: Needs work » Needs review

Those review comments were from me and as far as I know they are fixed. Setting to NR to review the MR.

jonathan1055’s picture

If the changes are fixed, maybe you could add a comment and resolve the thread, then other reviewers will know that these things are not still outstanding. The closed threads can be viewed, and even re-opened, but it helps if they are closed when it it believed that no more changes are needed on them. I'm sure you know all this anyway, but just posting here for new-comers, as this project has not had Merge Requests and pipelines before :-)

quietone’s picture

Issue summary: View changes

I resolved the threads. Can't others resolve them?

jonathan1055’s picture

Thanks for resolving. That make is easy to see there is currently nothing remaining.

I resolved the threads. Can't others resolve them?

No, not everyone can. I've not worked out the rules fully, but I think it depends a few things (a) who opened the MR (b) who is a maintainer of the project (c) who started the thread. In general I try to close/resolve them if it was me who started the query/review and the issue has been address and I have the permission to close it.

borisson_’s picture

I'd like to think this is mostly resolved now, we should probably check if all the page have been converted? Have there been edits in the time since we created this?

jonathan1055’s picture

Issue summary: View changes

Have there been edits in the time since we created this?

That's a good question. I have changed the issue summary tasks into a numbered list and added a new one for this. We can now refer to the steps by number and re-order if necessary.

Given that the pages will be created by the "pages" job in GitlabCI I do not quite understand why leaving that taskl is put right at the end. How will the pages be generated if we don't use the pre-supplied "pages" job?

quietone’s picture

Issue summary: View changes

I get notifications for changes to all the coding standard pages. And there haven't been any recent changes. And most of the pages have a note at the top explaining that changes are to be approved by the coding standard committee.

In the task list I think turning off editing of the d.o pages should be last, and have changed the order. I still think this should be in committed ASAP so we see the results and check links etc.

I do keep forgetting that not everyone can resolve a thread. That remains, well, frustrating.

quietone’s picture

I've cleaned up the many whitespace errors.

Unfortunately, I can not commit to this project. I'll have to figure that out.

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work

I commit this but there isn't the automated message. Why is that?

The next bit is to get GitLab pages generated.

jonathan1055’s picture

Here is the commit
https://git.drupalcode.org/project/coding_standards/-/commit/6e7ecb58dca...

I think the reason there is no automated comment is that the commit message text was not of the required format (starting Issue #nnnnnnn

The next bit is to get GitLab pages generated.

In #14 I talked about making the pages, but in #15 you suggested that should be in a follow-up. Happy to help there, when that issue has been created, or continue here if you want.

dww’s picture

I commit this but there isn't the automated message. Why is that?

Because the commit message wasn’t formatted at all, there’s no issue NID (nor credits / attributions) and so there’s no way to know what issue to put the automated message in.

I hate the default commit message provided by the d.o UI, but at least it includes a NID so that the git history points to the issue history. 😅

Might be worth reverting the commit and doing it again with a properly formatted message.

Since there are no releases, I wonder if we could even get a force push allowed here so we could remove the noise from the canonical history (especially so early in the repo’s life)…

  • quietone committed f4f92918 on main
    Issue #3521924 by quietone, borisson_, dww, jonathan1055: Convert Coding...
quietone’s picture

Status: Needs work » Fixed

Sorry about that. Yes, starting over is a good idea.

I copied over .gitlab-ci.yml, .gitignore and the mkdocs.yml from the governance project because that is using GitLab pages. I modified them as needed for this project.

I am going to set this to fixed and we can pick up the work in new issues. The first of which will be to get the theme working on the GitLab pages. I have read the docs and am not sure how to fix it. :-(

Thanks folks for getting this step done.

jonathan1055’s picture

Given that MR1 was not actually merged, I have just rebased it and pushed a change to run the cspell job, and not skip the others either, just to see which jobs run. Some of them should not, as we don't have .php files etc. It makes sense to re-use this MR rather than create a new issue and new MR if this one does the work necessary.

jonathan1055’s picture

Status: Fixed » Needs review

MR1 is ready for review and feedback. The four validation/linting jobs that are run are:

  • composer-lint - this checks the overall syntax of files, there is no harm in running it. But it may not be needed and just wasting resource, so could skip it.
  • cspell - we want this
  • eslint - this checks the formatting of .yml files, so we should keep this
  • phpcs - this is probably checking the .md files for standards. It passes, so is OK. But we may decide to skip it

The two validation/linting jobs which do not get run are phpstan (we have no .php files) and stylelint (we have no .css files)

quietone’s picture

I am not following why there is more work here since this has been committed and fixed. Why not use a separate issue?

jonathan1055’s picture

Status: Needs review » Fixed

OK. I've put this back to fixed. Then please can you close MR1 as it was not merged, and I will open MR2 on a new issue. That just seemed like make-work as we have all the infrastucture (issue branch and MR) right here, that's why I continued to use it. But no worries, I will do that.

jonathan1055’s picture

quietone’s picture

@jonathan1055, Thanks for moving to a new issue. I was working late the other day, that would be why I forgot to close the MR.
@dww, thanks

jonathan1055’s picture

Regarding the comments earlier about there being no automatic commit comment due to the lack of formatting of the commit message, if the Merge Request had been actually merged here (using the "merge" button at the end of the issue thread) then it does take the automatic message from the text box and created a commit comment. That would also have saved the effort of manually closing it afterwards.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

Issue summary: View changes

The last three items from the issue summary will be done elsewhere.