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

beman.exemplar: change library status to under development (exemplar is a template library) #89

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

neatudarius
Copy link
Member

@neatudarius neatudarius commented Dec 20, 2024

Issues: bemanproject/beman#77

Expected README.md:
image

@neatudarius neatudarius marked this pull request as ready for review December 20, 2024 22:47
@neatudarius neatudarius changed the title Change library status to under development (exemplar is a template library) beman.exemplar: change library status to under development (exemplar is a template library) Dec 20, 2024
@RaduNichita RaduNichita self-requested a review December 20, 2024 22:55
@neatudarius neatudarius force-pushed the LIBRARY_STATUS_UPDATE branch from 4d2db3c to b168053 Compare December 20, 2024 23:20
@neatudarius
Copy link
Member Author

@wusatosi , what should I do here?
image
I didn't change these lines. And I want to keep them! (collapsable menus in docs).

@wusatosi
Copy link
Member

@wusatosi , what should I do here?
image
I didn't change these lines. And I want to keep them! (collapsable menus in docs).

Hum, weird, inline HTML check should have been disabled. I will look into this.

@neatudarius
Copy link
Member Author

@wusatosi , what should I do here?
image
I didn't change these lines. And I want to keep them! (collapsable menus in docs).

Hum, weird, inline HTML check should have been disabled. I will look into this.

OK. Then I will proceed with the PR, as this behaviour is not from changed lines.

Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

LGTM!

README.md Outdated
Comment on lines 7 to 9
<!-- markdownlint-disable -->
<img src="https://github.com/bemanproject/beman/blob/main/images/logos/beman_logo-beman_library_under_development.png" style="width:5%; height:auto;"> ![Continuous Integration Tests](https://github.com/bemanproject/exemplar/actions/workflows/ci_tests.yml/badge.svg)
<!-- markdownlint-enable -->
Copy link
Member

@wusatosi wusatosi Dec 21, 2024

Choose a reason for hiding this comment

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

Is there a reason to use <img> html tag instead of markdown facility here?

markdown lint automatically ignore line length caused by urls, ideally ignore would not be needed here.

Also, can we put this two in separate lines?

Copy link
Member

@wusatosi wusatosi Dec 21, 2024

Choose a reason for hiding this comment

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

According to markdown-lint documentation.

Enable all rules: <!-- markdownlint-enable -->

This will cause the previously configured ignore rule to be flushed out.

Is there a reason we need to use this disable-enable pair?

If a ignore have to be used, consider: <!-- markdownlint-disable-next-line -->.

If ignore have to be multi-line, use the idiom suggested by markdownlint's documentation:

<!-- markdownlint-disable -->
any violations you want
<!-- markdownlint-restore -->

Copy link
Member

Choose a reason for hiding this comment

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

Why not simply do:

Suggested change
<!-- markdownlint-disable -->
<img src="https://github.com/bemanproject/beman/blob/main/images/logos/beman_logo-beman_library_under_development.png" style="width:5%; height:auto;"> ![Continuous Integration Tests](https://github.com/bemanproject/exemplar/actions/workflows/ci_tests.yml/badge.svg)
<!-- markdownlint-enable -->
![Library status](https://github.com/bemanproject/beman/blob/main/images/logos/beman_logo-beman_library_under_development.png)
![Continuous Integration Tests](https://github.com/bemanproject/exemplar/actions/workflows/ci_tests.yml/badge.svg)

Copy link
Member Author

Choose a reason for hiding this comment

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

Second syntax for including image doest not allow resize, at least not in standard Markdown on GitHub. You can simulate a change by editing a file in UI and do preview.

README.md Outdated Show resolved Hide resolved
Copy link
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

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

OK. Then I will proceed with the PR, as this behaviour is not from changed lines.

No. lint break is caused by changed lines.

Avoid fighting with markdownlint with ignore statements, ignore should be the last resort.

Put faith into the linting infrastructure, let's not have our code littered with lint-ignores.

@neatudarius
Copy link
Member Author

OK. Then I will proceed with the PR, as this behaviour is not from changed lines.

No. lint break is caused by changed lines.

Avoid fighting with markdownlint with ignore statements, ignore should be the last resort.

Put faith into the linting infrastructure, let's not have our code littered with lint-ignores.

I agree with any solution which can work right now. So what should I do if the CI complains the line is too long? And I actually want to insert a text with hyperlink? (The text is short, the hyperlink is long).

Currently, the infrastructure seems to not consider this case. If this is true, it should be solved in other issue/PR.

@JeffGarland JeffGarland self-assigned this Dec 23, 2024
Copy link

@JeffGarland JeffGarland left a comment

Choose a reason for hiding this comment

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

Overall this look good.

@neatudarius neatudarius force-pushed the LIBRARY_STATUS_UPDATE branch from ab62808 to b92614f Compare December 24, 2024 18:24
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@neatudarius neatudarius force-pushed the LIBRARY_STATUS_UPDATE branch from 0a443e5 to 508df8c Compare December 24, 2024 18:43
@wusatosi
Copy link
Member

OK. Then I will proceed with the PR, as this behaviour is not from changed lines.

No. lint break is caused by changed lines.
Avoid fighting with markdownlint with ignore statements, ignore should be the last resort.
Put faith into the linting infrastructure, let's not have our code littered with lint-ignores.

I agree with any solution which can work right now. So what should I do if the CI complains the line is too long? And I actually want to insert a text with hyperlink? (The text is short, the hyperlink is long).

Currently, the infrastructure seems to not consider this case. If this is true, it should be solved in other issue/PR.

It looks like markdown lint ignores length overflow due to url via []().

@neatudarius
Copy link
Member Author

It looks like markdown lint ignores length overflow due to url via []().

@wusatosi , would you agree with current PR state? Check rendered version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants