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

Feature/refactor + dependabot #32

Merged
merged 6 commits into from
Jul 3, 2024
Merged

Feature/refactor + dependabot #32

merged 6 commits into from
Jul 3, 2024

Conversation

CarlosGomes98
Copy link
Contributor

@CarlosGomes98 CarlosGomes98 commented Jul 2, 2024

  • Refactor directory structure to remove the unnecessary src directory
  • Set up dependabot
  • Added missing copyright headers (that is the bulk of the changes)

@CarlosGomes98
Copy link
Contributor Author

After merging, dependabot version updates should be enabled in the repo settings

@CarlosGomes98 CarlosGomes98 force-pushed the feature/dependabot branch 2 times, most recently from 81e727c to 102bb59 Compare July 2, 2024 12:09
@CarlosGomes98 CarlosGomes98 changed the title Feature/dependabot Feature/refactor + dependabot Jul 2, 2024
Copy link
Member

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 ?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, same thing.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

@Joao-L-S-Almeida Joao-L-S-Almeida Jul 2, 2024

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.

@Joao-L-S-Almeida
Copy link
Member

In general, how the massive renaming from src/terratorch to terratorch will affect the still open PR's (specially #19) ?

@CarlosGomes98
Copy link
Contributor Author

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

@Joao-L-S-Almeida
Copy link
Member

Joao-L-S-Almeida commented Jul 2, 2024

I believe just #19 may be affected by these changes. Maybe we could consider to approve or discard it before approving the current PR.
Since I have no more comments about this PR, I'll approve it as soon as this question is decided.

@Joao-L-S-Almeida Joao-L-S-Almeida merged commit 1ebeeb5 into main Jul 3, 2024
3 checks passed
@CarlosGomes98 CarlosGomes98 deleted the feature/dependabot branch October 2, 2024 08:31
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.

2 participants