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.

Sign up for GitHub

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

Jump to bottom

chore: Moved structures.py from tubular repository #34328

Merged
merged 3 commits into from
Mar 12, 2024
Merged

chore: Moved structures.py from tubular repository #34328

merged 3 commits into from
Mar 12, 2024

Conversation

farhan
Copy link
Contributor

@farhan farhan commented Mar 5, 2024 •
edited
Loading

@farhan farhan force-pushed the farhan/code-migrated-from-tubular-2 branch from 3b7bf5f to db81f00 Compare March 5, 2024 10:36
@farhan farhan force-pushed the farhan/code-migrated-from-tubular-2 branch from db81f00 to ab1451b Compare March 5, 2024 11:06
@farhan farhan marked this pull request as ready for review March 5, 2024 12:00
feanil
feanil reviewed Mar 5, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Generally looks good, but the name common is too generic in this context. As a second commit, can you move it so that the name of this set of scripts is structures_pruning

@farhan
Copy link
Contributor Author

farhan commented Mar 6, 2024

Generally looks good, but the name common is too generic in this context. As a second commit, can you move it so that the name of this set of scripts is structures_pruning

@feanil I used common to reside all the solo file scripts like structures.py. Otherwise, we may need to separate all the requirements, and readmes for all these short scripts.

WDYT?

@feanil
Copy link
Contributor

feanil commented Mar 11, 2024

From our synchronous conversation, I think we agreed to put this in its own folder because as much as possible we don't want to mix the dependencies for the different scripts together right now. So having a generic requirements file that has the dependencies for multiple scripts is less desirable than having separate requirements for each for now. If in the future we find there are too many different scripts and it increases our maintenance burden significantly, then we can consolidate where it makes sense. I think once you've updated the pathing on the script and testing, this will be ready for re-review.

@feanil feanil mentioned this pull request Mar 11, 2024
Closed
7 tasks
@farhan farhan force-pushed the farhan/code-migrated-from-tubular-2 branch from bc7af7e to 4822e95 Compare March 12, 2024 07:47
@farhan
Copy link
Contributor Author

farhan commented Mar 12, 2024 •
edited
Loading

From our synchronous conversation, I think we agreed to put this in its own folder because as much as possible we don't want to mix the dependencies for the different scripts together right now. So having a generic requirements file that has the dependencies for multiple scripts is less desirable than having separate requirements for each for now. If in the future we find there are too many different scripts and it increases our maintenance burden significantly, then we can consolidate where it makes sense. I think once you've updated the pathing on the script and testing, this will be ready for re-review.

@feanil Addressed, ready for next pass.
If all good, please squash and merge the PR.

@feanil feanil merged commit 7808913 into openedx:master Mar 12, 2024
67 checks passed
feanil added a commit to openedx-unsupported/tubular that referenced this pull request Mar 12, 2024
@feanil
feat: Deprecate the structures script.
7bc4a31 
This script has been copied into edx-platform in
openedx/edx-platform#34328 and so we want to
make sure we leave a warning here.
@feanil feanil mentioned this pull request Mar 12, 2024
Merged
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@ohnickmoy ohnickmoy mentioned this pull request Mar 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feanil feanil feanil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Aximprovements Team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@farhan @feanil @edx-pipeline-bot