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 reading java array type #216

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Fix reading java array type #216

merged 3 commits into from
Feb 2, 2024

Conversation

KasiaKoz
Copy link
Collaborator

@KasiaKoz KasiaKoz commented Dec 18, 2023

Fixes #220
The java array types were being read to a set with extra unnecessary characters.

E.g. given:

<attribute name="attrib" class="java.lang.Array">{'A', 'B'}</attribute>

would be read to

{"{'A'", " 'B'}"}

This is now fixed to give:

{"A", "B"}

Added a test to catch the problem:
Screenshot 2023-12-18 at 18 10 04

@KasiaKoz KasiaKoz requested a review from mfitz December 18, 2023 18:36
Copy link
Contributor

@mfitz mfitz left a comment

Choose a reason for hiding this comment

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

The bug fix looks good, but I've queried the conversion from list to set - are we okay with losing duplicate elements? Can this cause a problem?

Also - is there an issue for this bug?

CHANGELOG.md Show resolved Hide resolved
tests/test_input_matsim_reader.py Show resolved Hide resolved
@KasiaKoz
Copy link
Collaborator Author

KasiaKoz commented Jan 8, 2024

The bug fix looks good, but I've queried the conversion from list to set - are we okay with losing duplicate elements? Can this cause a problem?

right now we're ok and that's the expected behaviour, we have an issue: #125 so it's known and if this becomes a problem we can start a debate there on what should happen

Also - is there an issue for this bug?

No, I only noticed it when I looked at another bug, that was documented: #156

@KasiaKoz
Copy link
Collaborator Author

KasiaKoz commented Jan 8, 2024

Made an issue for the bug and updated the PR description

@KasiaKoz KasiaKoz requested a review from mfitz January 8, 2024 15:21
Copy link
Contributor

@mfitz mfitz left a comment

Choose a reason for hiding this comment

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

Cool.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (127fcf0) 92.08% compared to head (d0891cf) 92.08%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #216   +/-   ##
=======================================
  Coverage   92.08%   92.08%           
=======================================
  Files          37       37           
  Lines        6620     6620           
  Branches     1588     1588           
=======================================
  Hits         6096     6096           
  Misses        305      305           
  Partials      219      219           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KasiaKoz KasiaKoz merged commit 2aaab56 into main Feb 2, 2024
10 checks passed
@KasiaKoz KasiaKoz deleted the fix-reading-java-array branch February 2, 2024 13:33
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.

Java array type is not read as expected
3 participants