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.

Sign up for GitHub

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

Jump to bottom

fix: Yarn lint script #1012

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: Yarn lint script #1012

wants to merge 2 commits into from

Conversation

usantos-at-wiris
Copy link
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. Include, if necessary, a description of the proposed solution and the reasoning behind it.

This PR adds the broken functionality to yarn lint and also autofixes all posible errors

Type of Change

Please delete options that are not relevant.

  • Feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Chore (non-breaking change that doesn't add any functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (changes to documentation only)
  • Refactoring (non-breaking change that improves the code structure)

Checklist

  • My code follows the style guidelines of this project ( Run yarn lint to check)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings or errors
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • New and existing unit tests pass locally with my changes

How should be tested? (Manual or Automated Tests)

  • Manual tests: I have tested this changes manually.

@usantos-at-wiris usantos-at-wiris self-assigned this Oct 1, 2024
@usantos-at-wiris usantos-at-wiris added the type:bug Something isn't working label Oct 1, 2024
jgonzalez-at-wiris
jgonzalez-at-wiris requested changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@jgonzalez-at-wiris jgonzalez-at-wiris left a comment

Choose a reason for hiding this comment

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

Hay cambios que no me convencen del todo. Como tenemos algo delicado entre manos, retrasaría la fusión de esto a cuando estemos más tranquilo, por ejemplo tras la fusión de lo que estamos finalizando de ck5.

packages/devkit/src/mathml.js Outdated
@@ -201,7 +201,7 @@ export default class MathML {
// Otherwise without the space
if (mathml.indexOf(` class="wrs_${customEditor}"`) !== -1) {
return mathml.replace(` class="wrs_${customEditor}"`, "");
} else if (mathml.indexOf(`class="wrs_${customEditor}"`) !== -1) {
} if (mathml.indexOf(`class="wrs_${customEditor}"`) !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Este cambio no me inspira confianza (el código original tampoco, sinceramente). Revisemos que no estamos rompiendo nada.

packages/devkit/src/telemeter.js
@@ -1,4 +1,4 @@
import init, { Telemeter as TelemeterWASM } from "../telemeter-wasm";
import init, { Telemeter as TelemeterWASM } from "@wiris/telemeter-wasm";
Copy link
Contributor

Choose a reason for hiding this comment

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

Este cambio de import tampoco me inspira confianza. No sabía que el linter podía hacer algo tan "inteligente".

packages/devkit/telemeter-wasm/telemeter_wasm.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Juraría que este fichero es autogenerado, no me siento cómo cambiando nada del mismo aunque sean solo cosas de formato.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgonzalez-at-wiris jgonzalez-at-wiris jgonzalez-at-wiris requested changes

@carla-at-wiris carla-at-wiris Awaiting requested review from carla-at-wiris carla-at-wiris is a code owner

@xjiang-at-wiris xjiang-at-wiris Awaiting requested review from xjiang-at-wiris xjiang-at-wiris is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@usantos-at-wiris usantos-at-wiris

Labels
type:bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@usantos-at-wiris @jgonzalez-at-wiris