Skip to content

Conversation

@gabridele
Copy link
Collaborator

@gabridele gabridele commented Dec 20, 2024

Hi there,

I’m submitting this pull request to address a few issues I came across while using the library. Before submitting, I ran tests as outlined in the documentation to ensure everything works as expected.

Changes proposed in this pull request

This pull request aims to resolve the following issues:

Documentation that should be reviewed

Based on these changes, no updates to the existing documentation are needed.

Feel free to reach out if you have any questions or need further clarification on my edits!

closes #342
closes #341

@tien-tong tien-tong requested a review from tsalo January 8, 2025 17:54
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

I'd prefer to figure out what's going wrong with IsFile and PathExists than to replace them with Path. Can you drop those changes but keep the rest?

@gabridele gabridele reopened this Jan 14, 2025
@gabridele
Copy link
Collaborator Author

Hi,
I restaged all of my edits, thought it would be a cleaner way of doing it. Dropping those two commits and then forcefully pushing the edits created like 20 more commits. So I reset the head to match your main branch and re-committed the edits. I'm sure there's a less cumbersome way, but I'm not that experienced with PRs. Sorry about that

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I think you just need to link the associated issues with closes #... so that they close when we merge your PR.

@gabridele
Copy link
Collaborator Author

@tsalo done! added them in my first message

@tsalo
Copy link
Member

tsalo commented Jan 14, 2025

Perfect. Once CI passes, I'll merge.

@tsalo tsalo changed the title PR addressing issues I raised recently Add --force-unlock to copy-exemplars and fix path to script in apply Jan 14, 2025
@tsalo tsalo merged commit ad54b9c into PennLINC:main Jan 14, 2025
8 checks passed
@tien-tong
Copy link
Contributor

@gabridele does this PR close #340?

@tsalo
Copy link
Member

tsalo commented Jan 15, 2025

No, @gabridele dropped that element at my request.

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

Labels

None yet

Projects

None yet

3 participants