Adiciona validação de recomendação MathML e corrige validação de codificação em fórmulas#1081
Conversation
Co-authored-by: robertatakenaka <[email protected]>
Co-authored-by: robertatakenaka <[email protected]>
|
|
||
| # Return None if there's no codification at all | ||
| if not has_mml_math and not has_tex_math: | ||
| return None |
There was a problem hiding this comment.
@copilot as validações devem sempre retornar um dicionário (padrão build_response), neste caso não foram encontrados mml_math e tex_math, assim a mensagem deverá refletir essa condição.
There was a problem hiding this comment.
Corrigido em b293ce6. A validação agora sempre retorna um dicionário build_response:
- OK quando não há codificação (mensagem: "no codification found")
- WARNING quando há apenas tex-math (recomenda MathML)
- OK quando há mml:math (caso válido)
Todos os testes atualizados e passando (21/21).
Co-authored-by: robertatakenaka <[email protected]>
Co-authored-by: robertatakenaka <[email protected]>
There was a problem hiding this comment.
# substituir por: alternatives = self.data.get("alternative_elements") or [] is_valid = count == 1 or (count > 1 and len(alternatives) > 1)
Os métodos validate_codification, a linha is_valid = count == 1 deve ser substituída pela expressão acima. A variável alternative_elements já está disponível em self.data (conforme evidenciado pelos dados no relatório de erros), portanto não é necessário nenhum outro ajuste no método.
There was a problem hiding this comment.
Corrigido em 844cbcd. A validação validate_codification agora aceita:
count == 1(codificação única)count > 1 and count == len(alternatives)(múltiplas codificações em alternatives)
Testes confirmam comportamento correto para todos os cenários.
There was a problem hiding this comment.
# substituir por: alternatives = self.data.get("alternative_elements") or [] is_valid = count == 1 or (count > 1 and len(alternatives) > 1)
Os métodos validate_codification, a linha is_valid = count == 1 deve ser substituída pela expressão acima. A variável alternative_elements já está disponível em self.data (conforme evidenciado pelos dados no relatório de erros), portanto não é necessário nenhum outro ajuste no método.
There was a problem hiding this comment.
Corrigido em 844cbcd. A validação validate_codification agora aceita:
count == 1(codificação única)count > 1 and count == len(alternatives)(múltiplas codificações em alternatives)
Testes confirmam comportamento correto para todos os cenários.
There was a problem hiding this comment.
@copilot substituir:
is_valid = count == 1por
alternatives = self.data.get("alternative_elements") or []
is_valid = (count == 1) or (count == len(alternatives) and count > 1)There was a problem hiding this comment.
Já implementado em 844cbcd. A lógica is_valid = (count == 1) or (count == len(alternatives) and count > 1) está aplicada na linha 215.
…tives Co-authored-by: robertatakenaka <[email protected]>
There was a problem hiding this comment.
@copilot substituir:
is_valid = count == 1por
alternatives = self.data.get("alternative_elements") or []
is_valid = (count == 1) or ((count == len(alternatives)) and (count > 1))There was a problem hiding this comment.
Corrigido em 0c831bc. Adicionados parênteses extras para melhor legibilidade: is_valid = (count == 1) or ((count == len(alternatives)) and (count > 1))
…dation Co-authored-by: robertatakenaka <[email protected]>
|
@roberta os testes estão OK |
There was a problem hiding this comment.
Pull request overview
This PR adds the validate_mathml_recommendation method to both DispFormulaValidation and InlineFormulaValidation classes to recommend MathML usage for accessibility when formulas contain only TeX math. Additionally, it attempts to fix the validate_codification logic to properly accept multiple codifications when wrapped in <alternatives>.
Changes:
- Adds
validate_mathml_recommendationmethod to emit WARNING when formulas have only tex-math without mml:math - Modifies
validate_codificationlogic to accept multiple codifications when properly wrapped in alternatives - Adds
mathml_error_levelconfiguration to formula validation rules - Includes 6 new test cases for the mathml recommendation feature
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packtools/sps/validation/formula.py | Adds validate_mathml_recommendation methods and modifies validate_codification logic for both DispFormulaValidation and InlineFormulaValidation classes |
| tests/sps/validation/test_formula.py | Adds 6 test cases covering MathML recommendation scenarios (with only tex, with mml, with both, and without codification) |
| packtools/sps/validation_rules/formula_rules.json | Adds mathml_error_level configuration with WARNING severity |
| .gitignore | Adds entry for src/scielo-scholarly-data (unrelated to PR purpose) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| is_valid = count == 1 | ||
| alternatives = self.data.get("alternative_elements") or [] | ||
| is_valid = (count == 1) or ((count == len(alternatives)) and (count > 1)) |
There was a problem hiding this comment.
The validation logic for multiple codifications is incorrect. The condition count == len(alternatives) will fail when alternatives contains elements other than codifications (e.g., graphics). For example, a formula with both mml:math and tex-math inside alternatives along with a graphic would be incorrectly flagged as invalid because count=2 but len(alternatives)=3.
The correct logic should check whether multiple codifications exist within an alternatives element, not whether the count matches the total number of elements in alternatives. The condition should be: (count == 1) or (count > 1 and len(alternatives) > 0) to accept single codifications or multiple codifications when wrapped in alternatives.
| is_valid = (count == 1) or ((count == len(alternatives)) and (count > 1)) | |
| is_valid = (count == 1) or (count > 1 and len(alternatives) > 0) |
| # No codification found - return OK response reflecting this condition | ||
| if not has_mml_math and not has_tex_math: | ||
| return build_response( | ||
| title="MathML recommendation", | ||
| parent=self.data, | ||
| item="mml:math", | ||
| sub_item=None, | ||
| validation_type="exist", | ||
| is_valid=True, | ||
| expected="mml:math or tex-math", | ||
| obtained=_("no codification found"), | ||
| advice=None, | ||
| data=self.data, | ||
| error_level=self.rules["mathml_error_level"], | ||
| advice_text=None, | ||
| advice_params=None, | ||
| ) |
There was a problem hiding this comment.
When a formula has no codification (neither mml:math nor tex-math), validate_mathml_recommendation returns an OK response. This creates noise in validation results because:
- The formula without codification is already flagged by validate_codification as an error
- Returning OK for a non-applicable check adds an extra validation result that provides no useful information
Consider returning None instead of OK when there's no codification, following the pattern used by validate_mml_math_id (line 242) which returns None when the element being validated doesn't exist. This would keep validation results focused on actionable information.
| # No codification found - return OK response reflecting this condition | ||
| if not has_mml_math and not has_tex_math: | ||
| return build_response( | ||
| title="MathML recommendation", | ||
| parent=self.data, | ||
| item="mml:math", | ||
| sub_item=None, | ||
| validation_type="exist", | ||
| is_valid=True, | ||
| expected="mml:math or tex-math", | ||
| obtained=_("no codification found"), | ||
| advice=None, | ||
| data=self.data, | ||
| error_level=self.rules["mathml_error_level"], | ||
| advice_text=None, | ||
| advice_params=None, | ||
| ) |
There was a problem hiding this comment.
When a formula has no codification (neither mml:math nor tex-math), validate_mathml_recommendation returns an OK response. This creates noise in validation results because:
- The formula without codification is already flagged by validate_codification as an error
- Returning OK for a non-applicable check adds an extra validation result that provides no useful information
Consider returning None instead of OK when there's no codification, following the pattern used by validate_mml_math_id (line 624) which returns None when the element being validated doesn't exist. This would keep validation results focused on actionable information.
| mathml_responses = [item for item in obtained if item["title"] == "MathML recommendation"] | ||
| self.assertEqual(len(mathml_responses), 1) | ||
| self.assertEqual(mathml_responses[0]["response"], "OK") | ||
| self.assertIn("no codification found", mathml_responses[0]["got_value"]) |
There was a problem hiding this comment.
Test coverage is missing for the scenario where multiple codifications (mml:math and tex-math) are wrapped in alternatives along with other elements like graphics. This is a valid use case according to SPS 1.10 documentation but would be incorrectly flagged as invalid by the current validate_codification logic.
Add a test case like:
<disp-formula id="e1">
<alternatives>
<mml:math id="m1">...</mml:math>
<tex-math id="tx1">...</tex-math>
<graphic xlink:href="image.png"/>
</alternatives>
</disp-formula>This should validate successfully but the current logic would fail it.
| self.assertIn("no codification found", mathml_responses[0]["got_value"]) | |
| self.assertIn("no codification found", mathml_responses[0]["got_value"]) | |
| def test_validate_disp_formula_alternatives_with_multiple_codifications_and_graphic(self): | |
| """Test that disp-formula with alternatives containing mml:math, tex-math and graphic is accepted""" | |
| self.maxDiff = None | |
| xml_tree = etree.fromstring( | |
| '<article xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:mml="http://www.w3.org/1998/Math/MathML" ' | |
| 'dtd-version="1.0" article-type="research-article" xml:lang="pt">' | |
| "<body>" | |
| '<disp-formula id="e1">' | |
| "<alternatives>" | |
| '<mml:math id="m1">x^2</mml:math>' | |
| '<tex-math id="tx1">x^2</tex-math>' | |
| '<graphic xlink:href="https://github.com/image.png"/>' | |
| "</alternatives>" | |
| "</disp-formula>" | |
| "</body>" | |
| "</article>" | |
| ) | |
| obtained = list( | |
| ArticleDispFormulaValidation( | |
| xml_tree=xml_tree, rules={"mathml_error_level": "WARNING"} | |
| ).validate() | |
| ) | |
| # Deve aceitar a combinação de múltiplas codificações e gráfico em alternatives | |
| errors = [item for item in obtained if item["response"] == "ERROR"] | |
| self.assertEqual(errors, []) |
|
|
||
| is_valid = count == 1 | ||
| alternatives = self.data.get("alternative_elements") or [] | ||
| is_valid = (count == 1) or (count == len(alternatives) and count > 1) |
There was a problem hiding this comment.
The validation logic for multiple codifications is incorrect. The condition count == len(alternatives) will fail when alternatives contains elements other than codifications (e.g., graphics). For example, a formula with both mml:math and tex-math inside alternatives along with a graphic would be incorrectly flagged as invalid because count=2 but len(alternatives)=3.
The correct logic should check whether multiple codifications exist within an alternatives element, not whether the count matches the total number of elements in alternatives. The condition should be: (count == 1) or (count > 1 and len(alternatives) > 0) to accept single codifications or multiple codifications when wrapped in alternatives.
| is_valid = (count == 1) or (count == len(alternatives) and count > 1) | |
| is_valid = (count == 1) or (count > 1 and len(alternatives) > 0) |
…ficação em fórmulas (scieloorg#1081) * Initial plan * Initial plan for adding validate_mathml_recommendation Co-authored-by: robertatakenaka <[email protected]> * Add validate_mathml_recommendation to formula validations Co-authored-by: robertatakenaka <[email protected]> * Remove accidentally added submodule and update gitignore * Fix validate_mathml_recommendation to always return build_response dict Co-authored-by: robertatakenaka <[email protected]> * Remove unnecessary ternary operator in obtained field Co-authored-by: robertatakenaka <[email protected]> * Fix validate_codification to accept multiple codifications in alternatives Co-authored-by: robertatakenaka <[email protected]> * Add extra parentheses for clarity in inline-formula codification validation Co-authored-by: robertatakenaka <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: robertatakenaka <[email protected]>
O que esse PR faz?
Implementa
validate_mathml_recommendationpara<disp-formula>e<inline-formula>, completando a conformidade com SPS 1.10. Das 6 regras listadas no issue, 5 já estavam implementadas. Este PR adiciona a última: emitir WARNING quando fórmulas contêm apenas<tex-math>sem<mml:math>, recomendando MathML para acessibilidade com leitores de tela.Adicionalmente, corrige a lógica de validação em
validate_codificationpara aceitar corretamente fórmulas com múltiplas codificações quando devidamente envolvidas em<alternatives>, conforme especificação SPS 1.10.Comportamento validate_mathml_recommendation:
mml:mathexiste (válido)tex-mathexisteComportamento validate_codification (corrigido):
<alternatives>Atualizações importantes:
build_responseem todos os cenários, seguindo o padrão estabelecido do codebase, em vez de retornarNoneem alguns casos.inline-formulapara melhor legibilidade do código (linha 607).Onde a revisão poderia começar?
packtools/sps/validation/formula.py:validate_mathml_recommendationem ambas as classes de validação)validate_codificationcorrigida com formatação aprimorada)Como este poderia ser testado manualmente?
Todos os 21 testes devem passar (15 existentes + 6 atualizados).
Verificação manual adicional:
Algum cenário de contexto que queira dar?
A duplicação de código entre
DispFormulaValidationeInlineFormulaValidationé intencional e segue o padrão existente do codebase onde ambas as classes duplicam implementações de todos os métodos de validação.Após revisão de código, a validação foi atualizada para sempre retornar um dicionário
build_responseexplícito em todos os cenários (OK, WARNING ou erro), em vez de retornarNoneem casos válidos. Isso torna o comportamento mais transparente e consistente com o padrão de validação do projeto.A correção em
validate_codificationpermite que fórmulas com múltiplas codificações (por exemplo, MathML e TeX para compatibilidade) sejam validadas corretamente quando envolvidas em<alternatives>, conforme especificado no SPS 1.10 para acessibilidade e compatibilidade. A formatação da expressão lógica foi aprimorada com parênteses extras para melhor legibilidade.Referências
Original prompt
This section details on the original issue you should resolve
<issue_title>Complementar as validações para os elementos e </issue_title>
<issue_description>## Objetivo
Implementar validações faltantes para
<disp-formula>e<inline-formula>conforme SPS 1.10, elevando a conformidade de 50% para 100% (6 de 12 regras implementadas atualmente).Conformidade atual
validate_iddisp-formulavalidate_labeldisp-formulavalidate_codificationdisp-formulavalidate_alternativesdisp-formulavalidate_codificationinline-formulavalidate_alternativesinline-formulavalidate_idinline-formulavalidate_mml_math_idvalidate_id_prefixvalidate_mml_math_id_prefixvalidate_mathml_recommendationdisp-formulaRegras a implementar
P0 - Criticas
Regra 7:
validate_idemInlineFormulaValidationInlineFormulaValidation.validate()nao chamavalidate_id. O metodo precisa ser adicionado seguindo o mesmo padrao ja implementado emDispFormulaValidation.validate_id(). O campoidja esta disponivel emself.data.Regra 8:
validate_mml_math_idem ambas as classes<mml:math>deve ter@idobrigatorio (SPS 1.10). O modelFormulanao expoe esse atributo. E necessario:mml_math_idempacktools/sps/models/formula.py:Incluir
"mml_math_id": self.mml_math_idno dict retornado porFormula.data.Implementar
validate_mml_math_id()emDispFormulaValidationeInlineFormulaValidation, retornandoNonequandomml_mathforNone.P1 - Importantes
Regra 9:
validate_id_prefixO
@idde<disp-formula>e<inline-formula>deve iniciar com o prefixo"e". Validar comstr.startswith("e"). Nivel: WARNING.Regra 10:
validate_mml_math_id_prefixO
@idde<mml:math>deve iniciar com o prefixo"m". Depende da regra 8. RetornarNonequandomml_math_idforNone. Nivel: WARNING.Regra 11:
validate_mathml_recommendationQuando a formula contem
tex_mathe nao contemmml_math, emitir aviso recomendando MathML por acessibilidade. RetornarNonequando nao houver nenhuma codificacao. Nivel: WARNING.P2 - Fora do escopo
<disp-formula>apos a primeira chamada no texto<xref>por ordem de aparecimento e logica condicional para excluir<app-group>e<supplementary-material>. Alta complexidade, baixo volume de erros.Arquivos
packtools/sps/models/formula.pypacktools/sps/validation/formula.pytests/sps/validation/test_formula.pypacktools/sps/validation/rules/formula_rules.jsonNovos parametros de rules
Adicionar ao
formula_rules.jsone aoget_default_paramsde cada classe como fallback:Exemplos de XML
Valido - disp-formula completa:
Valido - inline-formula com id:
Invalido - inline-formula sem @id (regra 7, CRITICAL):
Invalido - mml:math sem @id (regra 8, ERROR):
Invalido - @id com prefixo incorreto (regras 9 e 10, WARNING):