Skip to content

Fix declaration of get_miniscope_root_data_dir - #54

Merged
kushalbakshi merged 2 commits into
datajoint:mainfrom
MilagrosMarin:main
Jan 22, 2024
Merged

Fix declaration of get_miniscope_root_data_dir#54
kushalbakshi merged 2 commits into
datajoint:mainfrom
MilagrosMarin:main

Conversation

@MilagrosMarin

@MilagrosMarin MilagrosMarin commented Jan 17, 2024

Copy link
Copy Markdown
Collaborator

This PR initially aimed to correct minor typos, changing imaging_root_data_dir to miniscope_root_data_dir in tutorial_pipeline.py.

Following the review, a discussion has sparked on our approach to version and CHANGELOG updates across open-source repositories. The version and CHANGELOG in this PR have been manually updated for the time being, pending forthcoming documentation that will guide future version and changelog updates in subsequent PRs.

@kushalbakshi kushalbakshi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Please update the version and CHANGELOG.

@MilagrosMarin

Copy link
Copy Markdown
Collaborator Author

These modifications don't require a version update.

@kushalbakshi

Copy link
Copy Markdown
Contributor

These modifications don't require a version update.

Is not a patch release?

@MilagrosMarin

Copy link
Copy Markdown
Collaborator Author

These modifications don't require a version update.

Is not a patch release?

Based on internal agreement, there is no requirement to release a new version for these changes.

@kushalbakshi

Copy link
Copy Markdown
Contributor

These modifications don't require a version update.

Is not a patch release?

Based on internal agreement, there is no requirement to release a new version for these changes.

I haven't agreed to that. In the future please do one of the following:

  • update the version on any PR to be merged to the main branch - THIS IS SOP.
  • Explain why we're deviating from standard procedures in the PR description.
  • Only request reviews from reviewers with whom you've previously agreed to deviate from standard procedures to wrap up the PR as quickly as possible

@MilagrosMarin

Copy link
Copy Markdown
Collaborator Author

These modifications don't require a version update.

Is not a patch release?

Based on internal agreement, there is no requirement to release a new version for these changes.

I haven't agreed to that. In the future please do one of the following:

  • update the version on any PR to be merged to the main branch - THIS IS SOP.
  • Explain why we're deviating from standard procedures in the PR description.
  • Only request reviews from reviewers with whom you've previously agreed to deviate from standard procedures to wrap up the PR as quickly as possible

Thank you for underscoring the importance of maintaining clear practices for our open-source repositories. Here's a concise update on the ongoing discussion:

Initially, following guidance from our Slack channel, we determined that a version bump isn't necessary after a PR review in notebooks, README, and tutorial_pipeline.py.

However, this PR conversation has sparked an ongoing discussion on best practices for versioning and changelog management, with forthcoming documentation from @kushal. Additionally, we are exploring the adoption of the conventional commit vscode extension.

Given that this discussion is still pending resolution, I plan to manually bump the CHANGELOG and version following Kushal's guidance, and will include these details in the PR description. We remain open to considering alternative methods, such as the potential auto-generation of the CHANGELOG using VSCode extensions.

@kushalbakshi kushalbakshi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @MilagrosMarin!

@kushalbakshi
kushalbakshi merged commit dd3597f into datajoint:main Jan 22, 2024
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