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

Unify the drawing of badges #2220

Merged
merged 13 commits into from
Jan 6, 2023
Merged

Conversation

bdovaz
Copy link
Collaborator

@bdovaz bdovaz commented Jan 4, 2023

  • Badges now are generated in all places where the linter is mentioned (descriptor, individual descriptor and flavor)
  • Unify the generation of badges in a single function: get_badges(linter)
  • Add deprecated linter badge to get_badges(linter)
  • Add missing linter_repo fields: In the get_repository_badge_url(linter) method it takes into account and tries to search in the linter_url field if the linter_repo field does not exist but as it is not a unified logic everywhere, I have added the fields in the missing linters

bdovaz added 3 commits January 5, 2023 00:33
- Badges are generated in all places where the linter is mentioned (descriptor, individual descriptor and flavor)
- Unify the generation of badges in a single function
- Add deprecated linter badge
@bdovaz bdovaz requested a review from nvuillam as a code owner January 4, 2023 23:54
@nvuillam
Copy link
Member

nvuillam commented Jan 5, 2023

That seems to be a nice refactoring PR :)

But I (may) remember that for some doc pages, having less badges was on purpose to avoid ugly displays in tables

Please can you try locally with mkdocs serve and check that the doc display is still correct ?

pip install --upgrade "markdown==3.3.7" mike mkdocs-material mdx_truly_sane_lists jsonschema json-schema-for-humans giturlparse webpreview github-dependents-info
mkdocs serve

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2023

Codecov Report

Merging #2220 (d363ded) into main (172d5d6) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2220      +/-   ##
==========================================
+ Coverage   82.48%   82.50%   +0.02%     
==========================================
  Files         170      170              
  Lines        4470     4470              
==========================================
+ Hits         3687     3688       +1     
+ Misses        783      782       -1     
Impacted Files Coverage Δ
megalinter/reporters/UpdatedSourcesReporter.py 89.74% <0.00%> (+2.56%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 5, 2023

That seems to be a nice refactoring PR :)

But I (may) remember that for some doc pages, having less badges was on purpose to avoid ugly displays in tables

Please can you try locally with mkdocs serve and check that the doc display is still correct ?

pip install --upgrade "markdown==3.3.7" mike mkdocs-material mdx_truly_sane_lists jsonschema json-schema-for-humans giturlparse webpreview github-dependents-info
mkdocs serve

Doing what you say, the only thing I see that looks bad is the last commit because the text is so long that it looks very small:

image

We can override the "last commit" text and leave it blank:

image

We can do this only for the cases where we know it looks bad or for all cases or directly remove this badge on certain pages but I like less the solution. The link in that image takes you to https://github.com/{user}/{repo}/commits so it is understood when doing over what it is. I would like the <img /> tag to have an option to add the title attribute but the shields.io website doesn't allow it: https://shields.io/category/activity

What do you think @nvuillam?

@nvuillam
Copy link
Member

nvuillam commented Jan 5, 2023

The last commit is not so relevant... especially not in first position ... maybe just remove it ?

@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 5, 2023

The last commit is not so relevant... especially not in first position ... maybe just remove it ?

@nvuillam do you think so? The color of that particular badge gives at a glance the health of that linter in terms of maintenance. It is not feasible to periodically monitor the status of each linter one by one.

The information on that badge tells you things like if it is time to mark a linter as deprecated.

To appear on pages like this: https://megalinter.io/latest/supported-linters/ allows at a glance to visualize the health status of each linter in a very simple way.

If it is true that the fact that it appears first of all is not logical. What do we do then?

  1. We move it to the last position + the change I have made to shorten the name?
  2. We decide page by page (descriptor, individual descriptor, flavor...) whether to show it or not depending on how it is displayed (under the title or in table)?

I opt for the first one but well, I am open to other opinions.

@nvuillam
Copy link
Member

nvuillam commented Jan 5, 2023

I'm more team solution 2, because on the linter doc page it's great to have lots of badges (we could even add more !), but in tables we need a restricted one (deprecated if applicable, stars, formatter if applicable, autofix if applicable, SARIF if applicable)

For linter doc page, we could have:

  • same than table badges +
  • Github last release: GitHub release
  • Github last commit: GitHub last commit
  • Github commits activity: GitHub commit activity
  • Github contributors: GitHub contributors

@nvuillam
Copy link
Member

nvuillam commented Jan 5, 2023

While you're at it, if you can remove the column configuration key from https://megalinter.io/latest/supported-linters/ , it will make a better display ^^

@nvuillam
Copy link
Member

nvuillam commented Jan 5, 2023

(horrible invisible horizontal scrollbar in the table here :/ )

image

@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 5, 2023

While you're at it, if you can remove the column configuration key from https://megalinter.io/latest/supported-linters/ , it will make a better display ^^

@nvuillam I'm trying to do that but when I remove the "Configuration key" column from each page of a flavor, I get all the rows in the table disappearing.

I think it has to do with <!-- linter-icon --> because if I remove that it doesn't happen anymore. Can it be that when deleting a column, some calculation is made with some row that makes disappear what it should not? I know that in those pages there is some kind of logic to hide repeated icons, isn't it? Example: https://megalinter.io/latest/flavors/documentation/

@nvuillam
Copy link
Member

nvuillam commented Jan 5, 2023

If you remove from the table header + the element in the libe it should be ok :)

But don't worry I can do that in a PR after yours:)

bdovaz added 4 commits January 6, 2023 00:45
- Remove Configuration Key column
- Render configuration key value with linter name
- Render individual linter badges on a new line
- Unify all badges in one function
- Allow to hide some badges on certain pages
@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 5, 2023

@nvuillam I think that with this commit you would have everything you asked for, except for the flavors and <!-- linter-icon -->.

@nvuillam
Copy link
Member

nvuillam commented Jan 6, 2023

@bdovaz sorry I validated the dotnet v6 PR, lots of conflicts ^^

@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 6, 2023

@nvuillam conflicts resolved!

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

Great PR R by a great contrbutor, again :)

@nvuillam nvuillam merged commit 9a660be into oxsecurity:main Jan 6, 2023
@bdovaz bdovaz deleted the feature/more-badges branch January 6, 2023 16:10
@bdovaz
Copy link
Collaborator Author

bdovaz commented Jan 6, 2023

@nvuillam the problem of flavors tables being empty is still a problem, did you finally look at it?

Example:

https://megalinter.io/beta/flavors/dotnet/

@nvuillam
Copy link
Member

nvuillam commented Jan 7, 2023

woooo didn't see this one :o

@nvuillam
Copy link
Member

nvuillam commented Jan 7, 2023

@bdovaz found it :)

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.

3 participants