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

fix sonarcloud-reported bug in xmlwriter, add test #1951

Merged
merged 9 commits into from May 25, 2022
Merged

fix sonarcloud-reported bug in xmlwriter, add test #1951

merged 9 commits into from May 25, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 17, 2022

Summary of changes

sonarcloud detected a bug: “Remove 1 unexpected arguments; 'join' expects 1 positional arguments.” in xmlwriter:

return ":".join(pre, uri[len(ns) :])

Added containing brackets apparently omitted by original author.

return ":".join([pre, uri[len(ns) :]])

Added test for surety, took the opportunity of adding two tests to take coverage of xmlwriter to 100%.

Checklist

  • Checked that there aren't other open pull requests for the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes.
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

Graham Higgins added 3 commits May 17, 2022 11:37
“Remove 1 unexpected arguments; 'join' expects 1 positional arguments.”
@ghost ghost self-requested a review May 17, 2022 10:47
@ghost
Copy link
Author

ghost commented May 17, 2022

This might be getting a bit out of hand

test/test_serializers/test_xmlwriter_qname.py
    10:   6 N806 variable 'EXNS' in function should be lowercase [pep8-naming]
    11:   6 N806 variable 'TRIXNS' in function should be lowercase [pep8-naming]
    36:   6 N806 variable 'EXNS' in function should be lowercase [pep8-naming]
    37:   6 N806 variable 'TRIXNS' in function should be lowercase [pep8-naming]
    71:   6 N806 variable 'EXNS' in function should be lowercase [pep8-naming]
    72:   6 N806 variable 'TRIXNS' in function should be lowercase [pep8-naming]

@coveralls
Copy link

coveralls commented May 17, 2022

Coverage Status

Coverage increased (+0.01%) to 88.447% when pulling 464aa99 on gjhiggins:fix-other-sonarcloud-reported-bug into 32979d1 on RDFLib:master.

@aucampia
Copy link
Member

This might be getting a bit out of hand


test/test_serializers/test_xmlwriter_qname.py

    10:   6 N806 variable 'EXNS' in function should be lowercase [pep8-naming]

    11:   6 N806 variable 'TRIXNS' in function should be lowercase [pep8-naming]

    36:   6 N806 variable 'EXNS' in function should be lowercase [pep8-naming]

    37:   6 N806 variable 'TRIXNS' in function should be lowercase [pep8-naming]

    71:   6 N806 variable 'EXNS' in function should be lowercase [pep8-naming]

    72:   6 N806 variable 'TRIXNS' in function should be lowercase [pep8-naming]

But should these not be globals anyway? They look the same in all test functions.

Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

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

LGTM, nice with 100% test coverage.

@aucampia aucampia requested a review from a team May 17, 2022 13:05
@aucampia aucampia added the review wanted This indicates that the PR is ready for review label May 17, 2022
@aucampia
Copy link
Member

Will merge by 2022-05-25 if there is no further feedback.

@aucampia
Copy link
Member

pre-commit.ci autofix

@aucampia
Copy link
Member

pre-commit.ci autofix

@aucampia aucampia merged commit 685dec5 into RDFLib:master May 25, 2022
@ghost ghost deleted the fix-other-sonarcloud-reported-bug branch May 26, 2022 10:02
@ghost ghost removed the review wanted This indicates that the PR is ready for review label Jun 4, 2022
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.

None yet

2 participants