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

[#586] implement xml_mode for temporary parser changes #589

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

d-w-moore
Copy link
Collaborator

No description provided.

Copy link
Member

@trel trel left a comment

Choose a reason for hiding this comment

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

looks real nice.

irods/helpers/__init__.py Outdated Show resolved Hide resolved
irods/helpers/__init__.py Outdated Show resolved Hide resolved
irods/test/helpers.py Outdated Show resolved Hide resolved
irods/test/helpers.py Outdated Show resolved Hide resolved
irods/test/helpers.py Outdated Show resolved Hide resolved
irods/test/data_obj_test.py Outdated Show resolved Hide resolved
irods/test/data_obj_test.py Outdated Show resolved Hide resolved
irods/message/__init__.py Show resolved Hide resolved
irods/helpers/__init__.py Outdated Show resolved Hide resolved
irods/helpers/__init__.py Outdated Show resolved Hide resolved
irods/helpers/__init__.py Outdated Show resolved Hide resolved
irods/helpers/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

My comments are all minor improvements. Changes look real good

irods/test/data_obj_test.py Show resolved Hide resolved
irods/test/data_obj_test.py Outdated Show resolved Hide resolved
irods/test/data_obj_test.py Outdated Show resolved Hide resolved
irods/test/data_obj_test.py Outdated Show resolved Hide resolved
irods/helpers/__init__.py Outdated Show resolved Hide resolved
@alanking
Copy link
Contributor

Created a couple new issues for some of my comments. I left the others unresolved in case there was more to do, but they look like they can be resolved to me.

@trel
Copy link
Member

trel commented Jul 24, 2024

so i think we're ready to squash em, no #s.

@alanking
Copy link
Contributor

Yes, agreed. Consider addressing Codacy comments as well. Some unused / re-imported imports, apparently.

@d-w-moore
Copy link
Collaborator Author

so i think we're ready to squash em, no #s.

Squashed . Do we want mention of with xml_mode(...): in the README?

@alanking
Copy link
Contributor

Good point. Yes, let's document that. I think this section would be a good place to mention it, perhaps: https://github.com/irods/python-irodsclient?tab=readme-ov-file#special-characters

@d-w-moore
Copy link
Collaborator Author

README modification and additions have been made

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

New wording looks good to me. I had one suggestion

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Good. I think this is ready for squashing? Are tests passing?

@d-w-moore
Copy link
Collaborator Author

Tests are passing.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Great, # it

@alanking
Copy link
Contributor

alanking commented Aug 1, 2024

@d-w-moore - # when ready. Or, please let us know of what else needs to be done for this PR. Thanks

@d-w-moore
Copy link
Collaborator Author

# when ready. Or, please let us know of what else needs to be done for this PR. Thanks

Done! Pound added.

@alanking alanking merged commit 0c9600e into irods:main Aug 1, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants