Skip to content

Conversation

@kushalbakshi
Copy link
Collaborator

No description provided.

@kushalbakshi
Copy link
Collaborator Author

I'm not sure why it's resulting in the error for test-changelog. I'm not very familiar with semantic versioning and maybe I've missed something. I would appreciate any input to address the error

@kabilar
Copy link
Collaborator

kabilar commented Oct 10, 2022

Thanks @kushalbakshi. Is this formatted with black?

@kushalbakshi
Copy link
Collaborator Author

Thanks @kushalbakshi. Is this formatted with black?

It was not. I have updated it in the latest commit. Just need to add the docstring explanation now

kushalbakshi and others added 2 commits October 10, 2022 14:45
@kabilar
Copy link
Collaborator

kabilar commented Oct 10, 2022

I'm not sure why it's resulting in the error for test-changelog.

The reason that this is failing is because of our new GitHub Actions that requires each version to link (with brackets) to the tag (if it will exist after the merge) or a diff between versions. See this changelog as an example. We haven't updated this repo's changelog to reflect the new GitHub Actions. Would you also be able to make this update?

@kushalbakshi
Copy link
Collaborator Author

I'm not sure why it's resulting in the error for test-changelog.

The reason that this is failing is because of our new GitHub Actions that requires each version to link (with brackets) to the tag (if it will exist after the merge) or a diff between versions. See this changelog as an example. We haven't updated this repo's changelog to reflect the new GitHub Actions. Would you also be able to make this update?

Yes, I'm now seeing that this repo does not have any existing tags. I can make a release for v0.2.1 and another comparing that to v0.3.0?

@kushalbakshi
Copy link
Collaborator Author

Hi @kabilar, I tried to update the CHANGELOG entry to reflect that of element-array-ephys but it's still failing. Not sure what I'm missing. This is the error stack I'm seeing:

Run guzman-raphael/changelog-reader-action@v5
Warning: No target version specified. Will try to return the most recent one in the changelog file.
Parse data
Error: No log entry found

Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
Copy link
Collaborator

@kabilar kabilar left a comment

Choose a reason for hiding this comment

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

Thanks @kushalbakshi! Great work. A couple of questions listed below and then we can merge.

For future reference, we try to employ a line length of 88 for readability and use PEP8 standards for naming variables without using abbreviations. That said there are plenty of locations in our codebase that don't abide by these guidelines, but we have these goals for improving our code readability.

@kushalbakshi
Copy link
Collaborator Author

Thanks @kushalbakshi! Great work. A couple of questions listed below and then we can merge.

For future reference, we try to employ a line length of 88 for readability and use PEP8 standards for naming variables without using abbreviations. That said there are plenty of locations in our codebase that don't abide by these guidelines, but we have these goals for improving our code readability.

Sounds good. Thank you. I will update the linter and Black formatter settings in VScode to trigger errors when these aren't met

kushalbakshi and others added 13 commits October 12, 2022 17:11
Co-authored-by: Dimitri Yatsenko <dimitri@datajoint.com>
Co-authored-by: Dimitri Yatsenko <dimitri@datajoint.com>
Co-authored-by: Dimitri Yatsenko <dimitri@datajoint.com>
Co-authored-by: Dimitri Yatsenko <dimitri@datajoint.com>
Co-authored-by: Dimitri Yatsenko <dimitri@datajoint.com>
Co-authored-by: Dimitri Yatsenko <dimitri@datajoint.com>
Co-authored-by: Dimitri Yatsenko <dimitri@datajoint.com>
Co-authored-by: Dimitri Yatsenko <dimitri@datajoint.com>
@kushalbakshi
Copy link
Collaborator Author

@dimitri-yatsenko @kabilar I've tested a successful ingestion on my local machine. Updates here and at element-calcium-imaging now allow a user to use PrairieView as their acquisition software for populating ScanInfo.

Copy link
Collaborator

@kabilar kabilar left a comment

Choose a reason for hiding this comment

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

Thanks @kushalbakshi! Great work.

@kabilar kabilar merged commit 2ac1cce into datajoint:main Oct 13, 2022
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.

4 participants