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

[EC203] Detect unoptimized image format #47

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ValentinLeTallec
Copy link

@ValentinLeTallec ValentinLeTallec commented May 30, 2024

@dedece35 dedece35 requested a review from jycr June 13, 2024 19:35
@dedece35
Copy link
Member

HI @jycr, as SVG expert, are you OK with this implementation ?
for me, all seems OK but if we accept this PR, all images except SVG will be prohibited. Is it that we want ?

@ValentinLeTallec
Copy link
Author

For info, I took inspiration from the python implementation of the rule, which use the same exclusion list.
As such, if change are necessary on the java rule implementation, then the python implementation will probably also need to be changed.

@jycr
Copy link
Contributor

jycr commented Jun 13, 2024

@dedece35:

all images except SVG will be prohibited. Is it that we want ?

see my comment: green-code-initiative/creedengo-rules-specifications#153 (comment)

@dedece35
Copy link
Member

@dedece35:

all images except SVG will be prohibited. Is it that we want ?

see my comment: green-code-initiative/ecoCode#153 (comment)

I understand that, we can't make this rule as a global rule ! Thus, we can't accept it. OK with it ?

@utarwyn
Copy link
Member

utarwyn commented Jun 14, 2024

Hello,
I'd also like to comment because this rule exists in the repository (EC31) and was originally targeting React/HTML. It authorizes various image formats to mitigate this problem. There's no need to recreate a new EC203 rule.

Please consult the documentation here and the PR green-code-initiative/creedengo-rules-specifications#269.

I see that another PR has been opened for this rule: #52, maybe we can merge both works?

Copy link

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@github-actions github-actions bot added the stale label Jul 15, 2024
@dedece35
Copy link
Member

Hello, I'd also like to comment because this rule exists in the repository (EC31) and was originally targeting React/HTML. It authorizes various image formats to mitigate this problem. There's no need to recreate a new EC203 rule.

Please consult the documentation here and the PR green-code-initiative/ecoCode#269.

I see that another PR has been opened for this rule: #52, maybe we can merge both works?

Hi @utarwyn,
I absolutely agree with you ... but EC31 is declared on ecocode-rules-specifications but not yet implemented on javascript plugin. Am I right ? I found this PR green-code-initiative/creedengo-javascript#46 ... is it in review ?

I think we can wait this PR 46 and thhen, we will do the work to merge EC203 into EC31 (for Python) and review the current PR next.
What do you think @utarwyn about it ?
Do you think you can review quickly this javascript PR ?

@dedece35 dedece35 requested review from dedece35 and utarwyn August 11, 2024 22:04
@github-actions github-actions bot removed the stale label Aug 12, 2024
Copy link

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@github-actions github-actions bot added the stale label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants