-
Notifications
You must be signed in to change notification settings - Fork 8
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
Removing from source control now works #80
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have one comment here (re: use of %Library.File) - generally the approach is sound.
Good catch re: the "*.xml" filter being wrong and the need to skip over the top level directory to just consider the specifically-mapped directories (to avoid hitting errors on e.g. .md files)
cls/SourceControl/Git/Utils.cls
Outdated
while (mappingCoverage '= ""){ | ||
|
||
set mappedRelativePath = $$$SourceMapping(mappingFileType, mappingCoverage) | ||
set mappedFilePath = ..FixSlashes(..TempFolder()_(mappedRelativePath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally: I'd suggest trying to use %Library.File methods (e.g., in this case, NormalizeDirectory) rather than writing a method like FixSlashes.
|
||
set mappingFileType = $order($$$SourceMapping("")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought: we don't require explicit mappings if you're loading via the package manager. Might be worth considering the behavior of ListItemsInFiles in this case as well. (Though it's much trickier... Happy to just not worry about it for now.)
Removing from source control now works as expected. We still don't seem to support removing individual files from Source Control in cases where entire packages were added to source control. A fix for this would be to add individual files to source control instead of the package. Not sure how this would change things though.
This PR closes #76 :
Please review this more extensively than usual. I might have missed some things because of my limited ObjectScript knowledge.