Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

magento/devdocs#5249: Markdown linting: Spaces after list markers (MD030). Part 01 #5707

Merged

Conversation

atwixfirster
Copy link
Contributor

@atwixfirster atwixfirster commented Oct 16, 2019

Purpose of this pull request

This pull request provides changes regarding Markdown linting: Spaces after list markers (MD030) rule in the folder: _includes.

Part 01

Affected DevDocs pages

  • ...

Links to Magento source code

  • ...

@devops-devdocs
Copy link
Collaborator

An admin must run tests on this PR before it can be merged.

@rogyar rogyar self-assigned this Oct 16, 2019
@atwixfirster atwixfirster changed the title magento/devdocs#5249: Markdown linting: Spaces after list markers (MD030) magento/devdocs#5249: Markdown linting: Spaces after list markers (MD030). Part 01 Oct 16, 2019
@rogyar rogyar added the Special achievement High-value contributions that earn higher rewards and special attention label Oct 16, 2019
Copy link
Contributor

@jeff-matthews jeff-matthews left a comment

Choose a reason for hiding this comment

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

I neglected to add the proper rule syntax in the description for #5249. We are not using the default 1 space configuration. We're using the following custom configuration:

rule 'MD030', :ul_single => 2, :ol_single => 1, :ul_multi => 2, :ol_multi => 1 

Sorry about that @atwixfirster. You'll either need to update your PR or close and start over.

@atwixfirster
Copy link
Contributor Author

Sorry about that @atwixfirster. You'll either need to update your PR or close and start over.

fixed

@jeff-matthews , could you please review?

Thank you!

Copy link
Contributor

@jeff-matthews jeff-matthews left a comment

Choose a reason for hiding this comment

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

It looks like you removed spaces entirely in some cases, which is incorrect.

  • Unordered lists require two spaces after the list marker.
  • Ordered lists require one space after the list marker.

Don't forget to run tests locally after making changes to verify you fixed all errors in a particular file.

@atwixfirster
Copy link
Contributor Author

Don't forget to run tests locally after making changes to verify you fixed all errors in a particular file.

I ran tests after changes that I've made.

It looks like you removed spaces entirely in some cases, which is incorrect.
I am rechecking this point.

Thanks, @jeff-matthews

@atwixfirster atwixfirster force-pushed the MD030-includes-folder01 branch from f6c256a to cceda4e Compare October 16, 2019 16:39
@atwixfirster
Copy link
Contributor Author

@jeff-matthews , could you please verify now.

Many thanks as always :)

Copy link
Contributor

@jeff-matthews jeff-matthews left a comment

Choose a reason for hiding this comment

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

Looks great @atwixfirster!

Try to resist the urge to change note classes in future PRs ;)

@jeff-matthews jeff-matthews merged commit f85bf78 into magento:MD030-integration Oct 16, 2019
@ghost
Copy link

ghost commented Oct 16, 2019

Hi @atwixfirster, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@atwixfirster
Copy link
Contributor Author

atwixfirster commented Oct 16, 2019

Try to resist the urge to change note classes in future PRs ;)

@jeff-matthews , you are the BOSS here but I want to inform you what regarding to Dmitry Shevtsov notice in #4908 we should use a short format for note classes.

Thank you!

@jeff-matthews
Copy link
Contributor

Yes of course. That's true, but per issue description in #5249, you should limit scope to resolving MD030 errors only. It'll make PR reviews faster and easier.

@atwixfirster
Copy link
Contributor Author

Yes of course. That's true, but per issue description in #5249, you should limit scope to resolving MD030 errors only. It'll make PR reviews faster and easier.

sure, BOSS

Thank you!

@jeff-matthews
Copy link
Contributor

@atwixfirster,

It appears that I made a mistake when planning the initiative for #5249. I failed to take into account how many PRs would be associated with this effort. Awarding a Special achievement label (30 points) for each of the 26 PRs that you created creates imbalance in the community and is contrary to the spirit and intent of the rewards program.

I apologize for the mistake, but I'm going to have to award you a single Special achievement label for all the PRs that you created related to this issue. The remaining will receive the Editorial label.

I'm reviewing all other MD linting issues and PRs to apply the same standard to other contributors. See #5720.

Thank you for your all of your contributions to devdocs. We truly appreciate it.

@atwixfirster
Copy link
Contributor Author

atwixfirster commented Oct 31, 2019

Awarding a Special achievement label (30 points) for each of the 26 PRs that you created creates imbalance in the community and is contrary to the spirit and intent of the rewards program.

I am totally agreed with this point.

The remaining will receive the Editorial label.

Agreed

but ...

I apologize for the mistake,

your apologies are not accepted because of

5707-01

Thanks, @jeff-matthews

@jeff-matthews
Copy link
Contributor

@atwixfirster,

I'm not denying that I made that announcement. I consider that announcement the mistake. I am taking responsibility.

@atwixfirster
Copy link
Contributor Author

I'm not denying that I made that announcement. I consider that announcement the mistake. I am taking responsibility.

Thank you @jeff-matthews !

Anyway that was a good chance to increase contributors points. ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Partner: Atwix partners-contribution PR created by Magento partner Special achievement High-value contributions that earn higher rewards and special attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants