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

Throw ERROR when file areas point at same file #769

Merged
merged 6 commits into from Nov 29, 2023
Merged

Conversation

al-niessner
Copy link
Contributor

🗒️ Summary

Check during file area load if the file is already "in use" and report an error if it is.

⚙️ Test Data and/or Report

Automated unit tests below all pass

♻️ Related Issues

Closes #755

Al Niessner added 2 commits November 21, 2023 13:56
In this case 3 of the 4 files reference the same file. Make sure that too many are not caught
@al-niessner al-niessner requested a review from a team as a code owner November 21, 2023 22:06
@al-niessner al-niessner marked this pull request as draft November 21, 2023 22:06
Copy link
Member

@nutjob4life nutjob4life left a comment

Choose a reason for hiding this comment

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

Oh wait … it's a draft

@nutjob4life nutjob4life self-requested a review November 21, 2023 22:34
@al-niessner
Copy link
Contributor Author

@nutjob4life

Sorry, I think my work flow is different than the rest of ya. Since I am my own beat deaf drummer - yeah I make Elaine look like a good dancer - that should not be too much of a surprise. Anyway, I like to use the PR for testing since it works so well here which means I do a lot of pushes to it before letting it go. Sorry that it tricks you into thinking it is time for a review when it is not.

@al-niessner al-niessner marked this pull request as ready for review November 23, 2023 00:41
@nutjob4life
Copy link
Member

@al-niessner no worries; it's a known and open question whether draft pull requests should send notifications. I wish GitHub would notify only when it's non-draft.

I'll try to pay attention to the "draft" status, but once it's ready, you may have to re-ping me to get my review. (I practice "inbox zero".)

Copy link
Member

@jordanpadams jordanpadams left a comment

Choose a reason for hiding this comment

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

I am slightly concerned with our memory utilization having another data object that is going to need to store references for every target we come across, but I guess that is a necessary evil without some complex logic to know when you can flush the data object. Let's move forward with this and see if we encounter additional issues.

@jordanpadams jordanpadams changed the title 755: error when file areas point at same file Fix error when file areas point at same file Nov 29, 2023
@jordanpadams jordanpadams changed the title Fix error when file areas point at same file Throw ERROR when file areas point at same file Nov 29, 2023
@jordanpadams jordanpadams merged commit 88f4b69 into main Nov 29, 2023
2 checks passed
@jordanpadams jordanpadams deleted the issue_755 branch November 29, 2023 17:27
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.

As a user, I want validate to throw an error when a file is being referenced by more than one label
3 participants