I would rather you add the code to stage the files rather than just have it always return the files if they are not changed. If the files are not changed it should not say it changed them.

It took me a minute, but I think you're referring to this log:

The simplest and most direct way to stage the files based on what you've said is to stage each file right before returning None, so I did that in 411d3b5. The downside to doing it here was that it required creating a new Repo() object for each file, since I can't update the method definition of update_file_w_version without a bunch of other changes.

@codejedi365 If it isn't the right direction, I can further tweak based on your thoughts. Thanks

Sorry, it seems I was unclear. I was not talking about the log message--I was talking about conceptually what the return value of apply_version_to_source_files() represents. The return value of the function identifies that these are the paths that were modified. Conceptually, relating to your use case, only the first run of semantic-release version should modify the files since they have not yet been updated with the next version therefore the function returns those paths. On the second execution, since the files have not been modified the function should not return those paths. That is the intended contract of the apply_version_to_source_files() function.

The implementation that I see to solve this problem revolves around the --no-commit option and the resulting logical flow that comes from that option being set. I think you should look a bit further down the workflow to where it chooses where to make the commit or not. Remember in order to commit, we have to have changes staged in the index, which means PSR is already staging the files in the workflow. This is why I don't think we need to add a staging to apply_version_to_source_files(). I anticipate that the solution is to insert a conditional around the commit decision code that will always go ahead and stage the files but conditionally commit them.

Now a note about following with the current design pattern, PSR has a GitProject class that is the PSR interface to the git module. This class is where all git operations take place and we abstract the nuances of the git module away from the PSR workflows. If you could take a look at GitProject.git_add() & GitProject.git_commit() and how it is used in the version command to come up with an implementation that makes PSR always stage the release commit files but conditionally creates a commit based on the
--commit/--no-commit command line option. That is how I would solve this issue.

@codejedi365 Thanks! What you recommended is where I started as well, but I ran into this headwind:

• version_declarations.update_file_w_version has a strict return type of Path | None from IVersionReplacer.
• Inside the implementation of update_file_w_version it returns None in various different situations; when a file doesn't exist, when the version isn't found, or when the next and existing content is the same. So I didn't feel comfortable inferring that we should git add a file if the function returned a None in the corresponding point in the list.

Based on your comment though, it seems like you're OK with me changing the IVersionReplacer return which would make this much easier to do, but requires touching more code/files (which is fine).

If that's fine with you, I'll work on that this week? I just pushed a very simple mockup of using an enum; if that's in the right direction I'll finish off the logic (still WIP, had to wrap up for the day) and write some tests. Thanks for all the feedback!

Ok so I continued my work here but I had to change the return of apply_version_to_source_files() to be a list of the new enums, and then handle that in the caller version.py; I started to frame this out in 5e0fb8f, wdyt so far? Next time I get a chance I'll do some functional testing and likely some unit tests too, just wanted to push it out there for a review sooner than later in case you aren't a fan

I'm gonna wrap this up sometime this week using the current enum direction

@JonZeolla, sorry I was feeling under the weather this week. I don't think the enum implementation is necessary. It's nice to track all the variations of why and how it stamped the versions in files but we don't do anything with that informational state. The function apply_version_to_source_files() was written as I wanted it with the function contract above. There is no need to change it.

All I expected that was necessary is to solve this issue is move the git add command above the conditional if related to the --commit option.

This solution solves your issue because in the first execution it stamps the files and then adds them to the index. On the second execution nothing is stamped but when the commit happens it still will commit the entire index which will include the files from the previous PSR execution.

What circumstance are you trying to solve with the enum state implementation?

Is it because if there are no changes then it won't make a commit?

I think that will only happen if you don't build/update a changelog in the second execution of PSR. Maybe we adjust your workflow to just make sure that the first execution does not build a changelog.

Or we could just evaluate if the index is not empty then we can create a version commit.

@codejedi365 Aha, so the enum allowed us to ensure that a file whose old content was the same as the new content would get re-added on the second run, as well as the first run. But if you're comfortable having it be added on the first run only, then you're right we only need to move that git_add() call up like here.

Also yes, for my situation I am skipping changelogs because I'm releasing a number of packages from a single monorepo and don't want the changelog to have confusing contents (pending monorepo support, which I've seen you work on so 👍👍 I will happily wait for that support).

Let me look at that code path and push my thoughts here in a bit

Ok, I have your thoughts pushed now. I also pushed a commit with the updated and working tests via the enum, just for posterity since I had it.

I'm gonna look at the tests next to ensure my issue doesn't recur

This PR is stale because it has not been confirmed or considered ready for merge by the maintainers but has been open 60 days with no recent activity. It will be closed in 10 days, if no further activity occurs. Please make sure to add the proper testing, docs, and descriptions of changes before your PR can be merged. Thank you for your contributions.

Sorry, @JonZeolla, I got busy. I will try to get this merged in the next week or two.

No problem @codejedi365 thank you

I would love to have some tests to make sure this doesn't recur, but I won't have an opportunity to look into that myself for a number of weeks :/

@JonZeolla, If you have time tomorrow, would you take a look at this and maybe try it out (download artifact from CI workflow). I modified a previous version stamp only test to validate that it will stage any files that are stamped with a version. This should resolve your environment. I did not make a test that fully executes uv and a double version execution but I felt that was overkill as it also would of been harder to validate after the second execution. In my point of view, the part that was missing for you was the staging and that is resolved and checked so we should be good to merge & release.

I did rebase and overwrite your branch however I do have a local copy of your changes if we needed them.

@codejedi365 I just went to test but the artifact was expired. It's a bit hard for me to stay on top of this (mostly because of the notifications getting lost in a sea of other notifications), and this seems to be going in the right direction so I have no reason to hold it up. I also agree, that it seems to have coverage for my scenario right now - ty for that.

@JonZeolla Thanks!

🎉 This PR has been published as part of v10.1.0 🎉

You can find more information about this release on the GitHub Releases page.