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

Fix declaration of get_miniscope_root_data_dir #54

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

MilagrosMarin
Copy link
Collaborator

@MilagrosMarin MilagrosMarin commented Jan 17, 2024

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.

Copy link
Collaborator

@kushalbakshi kushalbakshi left a comment

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
Collaborator Author

These modifications don't require a version update.

@kushalbakshi
Copy link
Collaborator

These modifications don't require a version update.

Is not a patch release?

@MilagrosMarin
Copy link
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
Collaborator

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
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.

Copy link
Collaborator

@kushalbakshi kushalbakshi left a comment

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
7 checks passed
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