shpaass / yafc-ce

Powerful Factorio calculator/analyser that works with mods
GNU General Public License v3.0
54 stars 20 forks source link

feat: opening project file via file association #152

Closed Yukinyaa closed 1 month ago

Yukinyaa commented 3 months ago
Yukinyaa commented 3 months ago

Will open after actually tested

shpaass commented 3 months ago

Will open after actually tested

You can make this PR a draft instead of closing it. I think thematically it fits more.

Yukinyaa commented 3 months ago

Okey dokey

shpaass commented 3 months ago

Merge branch 'master' into master

I ask you to rebase the branch with git rebase -i to squash merge commits and other commits that were superseded.

shpaass commented 3 months ago

In cbad734, what does - perf: comparing mean in the commit message?

Yukinyaa commented 3 months ago

In cbad734, what does - perf: comparing mean in the commit message?|

In 47d1f6e(changed due to rebase), null check is done before Path.GetFullPath is called.

Should have been more concise, sry😢

shpaass commented 3 months ago

implimentation -> implementation redundent -> redundant

To check the grammar, I usually paste the message in google docs to see if it detects anything.

shpaass commented 3 months ago

feat: opening project file via file association fix: actually test opening project file fix: remove log and redundent code

I have a suspicion that all three commits can be squashed into one because the first one adds logic, when the other two fix that logic to do what was expected. What do you think?

Yukinyaa commented 3 months ago

I have a suspicion that all three commits can be squashed into one because the first one adds logic, when the other two fix that logic to do what was expected. What do you think?

I thought you were talking about squashed merge. I'm OK with squashing all commits in this case. Will do. And also will check for obvious spelling/grammar mistakes. :)

shpaass commented 3 months ago

I thought you were talking about squashed merge.

I'll quote my initial message about that:

[...] to squash merge commits and other commits that were superseded

By other commits that were superseded I meant the commits that fix the new functionality that the PR was aimed to bring. For example, if you make a change that doesn't initially work so you have to fix it in the code that you introduced, then you squash the fixes before the PR, unless the fixes are important by themselves for the understanding of the code.

Yukinyaa commented 3 months ago

Thank you for the clarification.

I did not realize that was the intended meaning in the Contributor's Guide. I will definitely comply with the guidelines from now on. I'm still getting the hang of contributing to open source projects, and I appreciate this learning opportunity.

shpaass commented 3 months ago

I did not realize that was the intended meaning in the Contributor's Guide.

It's fine. This part was not in the Contributor's Guide because I want to keep the guide concise. The thing that could be misinterpreted in our case is the following line from it:

Please separate refactoring and the change of behavior into different commits, so it is easier to review the PR.

This part is about changing the already-existing code. In your current case, if you refactor the new code that the PR brings, then you can squash the refactors.

shpaass commented 3 months ago

changelog.md

you mean changelog.txt?

veger commented 3 months ago

oops, yes

shpaass commented 3 months ago

@Yukinyaa please add a short summary of the change to the changelog.

shpaass commented 3 months ago

Since there are things to change in this PR, and they have not been addressed for a while, I'm converting it to a Draft.

shpaass commented 1 month ago

Since there has been no edits to this PR for months and since the fork of @Yukinyaa does not allow edits, I've transferred the PR to a repo branch in order to continue the development.