-
Notifications
You must be signed in to change notification settings - Fork 40
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
Feature/refactor + dependabot #32
Conversation
After merging, dependabot version updates should be enabled in the repo settings |
81e727c
to
102bb59
Compare
Signed-off-by: Carlos Gomes <[email protected]>
Signed-off-by: Carlos Gomes <[email protected]>
Signed-off-by: Carlos Gomes <[email protected]>
Signed-off-by: Carlos Gomes <[email protected]>
Signed-off-by: Carlos Gomes <[email protected]>
102bb59
to
b427754
Compare
Signed-off-by: Carlos Gomes <[email protected]>
d6f68d4
to
0068b3a
Compare
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.
These datamodules are being imported but not used, right ?
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.
They are not being used, but they enable from terratorch.datamodules import
, rather than needing to go down to the module
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.
Same question asked for the other init file.
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.
Yes, same thing.
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.
In which circumstances we want to perform the tests for individual pushes ? As we have the rule of creating PR before sending pushes to main
I thought it was already covered by the pull request instruction.
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.
Its a good point. Tbh I mostly took this from what I saw on TorchGeo. Could it be for the case where a force push to main happens?
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.
Right. I believe it won't affect our current workflow, but it could cover the exceptional cases in which we need to directly push to the master. I agree.
In general, how the massive renaming from src/terratorch to terratorch will affect the still open PR's (specially #19) ? |
@Joao-L-S-Almeida This is a good point. Should we hold off on this one until the currently open PRs are merged, and then do it? This should help minimize any headaches from it |
I believe just #19 may be affected by these changes. Maybe we could consider to approve or discard it before approving the current PR. |
src
directory