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

[FIX] Number explicitly all cases in MRI field map section headers #323

Conversation

jhlegarreta
Copy link
Contributor

Number explicitly all cases in MRI field map section headers for the
sake of consistency.

effigies
effigies previously approved these changes Sep 5, 2019
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

So case 4 was numbered and the other three weren't? Seems straightforward. I suspect that during the translation somebody accidentally interpreted the #### as automatically numbering.

@jhlegarreta
Copy link
Contributor Author

@effigies That's right.

@effigies
Copy link
Collaborator

effigies commented Sep 5, 2019

Style failure:

462:1-462:69  warning  Use headings shorter than `60`  maximum-heading-length  remark-lint

This is a bad requirement, IMO. I think we should probably add:

["lint-maximum-heading-length", false]

to:

{
"plugins": [
"preset-lint-markdown-style-guide",
["lint-no-duplicate-headings", false],
["lint-list-item-indent", "tab-size"],
["lint-emphasis-marker", "consistent"],
["lint-maximum-line-length", false]
]
}

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Sep 5, 2019

This is a bad requirement, IMO. I think we should probably add:

Although excessively long headings should be avoided IMHO, the markup already requires a few characters, so I agree with what you propose.

I would be for doing it in another PR, then rebasing this one. Is this change something that requires to be discussed by a larger part of the community or shall I push the change now?

@effigies
Copy link
Collaborator

effigies commented Sep 5, 2019

Sure, go ahead and open a PR to update the linter rules. It'll take two reviewers to merge. I don't think it's likely to be controversial.

@jhlegarreta
Copy link
Contributor Author

Sure, go ahead and open a PR to update the linter rules. It'll take two reviewers to merge. I don't think it's likely to be controversial.

Done in #325.

@jhlegarreta jhlegarreta changed the title [FIX]: Number explicitly all cases in MRI field map section headers [FIX] Number explicitly all cases in MRI field map section headers Sep 5, 2019
@effigies
Copy link
Collaborator

effigies commented Sep 5, 2019

Can you go ahead and rebase this commit onto #325 in order to verify that it is the correct fix for this linting error?

effigies
effigies previously approved these changes Sep 5, 2019
Copy link
Collaborator

@franklin-feingold franklin-feingold left a comment

Choose a reason for hiding this comment

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

Thank you for catching this! May you please add your name to our contributors wiki to credit your contribution? For releases, we take our wiki and update our contributors appendix

@jhlegarreta
Copy link
Contributor Author

You're very welcome @franklin-feingold !

I had already added my name to the contributors wiki !

@sappelhoff
Copy link
Member

OKay, this PR is now ready for a rebase @jhlegarreta :-)

Number explicitly all cases in MRI field map section headers for the
sake of consistency.
@jhlegarreta jhlegarreta force-pushed the NumberExplicitlyMRIFieldmapDataSectionHeaders branch from 5507c85 to ba93922 Compare September 11, 2019 13:10
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Sep 11, 2019

ba93922 rebased on master @sappelhoff.

@sappelhoff
Copy link
Member

Thanks! BTW: I like the way you named your branch to make this PR 😉

@effigies
Copy link
Collaborator

The actual change is 7 days old, so I don't see a need to wait 5 days from the most recent commit in this case. Merging.

@effigies effigies merged commit 1fa7396 into bids-standard:master Sep 12, 2019
@jhlegarreta jhlegarreta deleted the NumberExplicitlyMRIFieldmapDataSectionHeaders branch September 12, 2019 14:38
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.

4 participants