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

#387 #398

Merged
merged 12 commits into from
Sep 28, 2023
Merged

#387 #398

merged 12 commits into from
Sep 28, 2023

Conversation

Cia15
Copy link
Contributor

@Cia15 Cia15 commented Sep 12, 2023

This is my proposed fix for the issue #387.
Using LibGit2.GitCommit() and LibGit2.message() I added the gitcommit message. However, I haven't written the test for this one. Any feedback would be welcomed. Have a nice day!

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

Why do we need the statistics dependency and the settings.json file?

For the tests: you said you haven't written any tests, but did you try out this PR locally and it did work, i.e, it solved the #387 ?

"""
gitpatch(gitpath = projectdir())

Generates a patch describing the changes of a dirty repository
""" Generates a patch describing the changes of a dirty repository
Copy link
Member

Choose a reason for hiding this comment

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

I am confused about this documentation string change. It is a mistake. The previous version was obviously clearer.

@@ -200,6 +199,7 @@ Dict{Symbol,Any} with 3 entries:
function tag!(d::AbstractDict{K,T};
gitpath = projectdir(), force = false, source = nothing,
storepatch::Bool = readenv("DRWATSON_STOREPATCH", false),
mssg::Bool = false
Copy link
Member

Choose a reason for hiding this comment

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

mssg is not a clear keyword. PLease name it something that conveys its purpose better just as commit_message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, thank you for your feedback. I already changed the mssg keyword to commit_message

@Datseris
Copy link
Member

In any case, you need to write tests for this PR, by going into the test folder, and modifying one of the existing tests. Pass the keyword in, the load the file, and then see that it has a commit message as field.

@Datseris
Copy link
Member

And also, thank you for the contribution!

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #398 (486df1a) into main (5667d46) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
+ Coverage   90.57%   90.65%   +0.08%     
==========================================
  Files           8        8              
  Lines         753      760       +7     
==========================================
+ Hits          682      689       +7     
  Misses         71       71              
Files Coverage Δ
src/saving_tools.jl 82.45% <100.00%> (+1.14%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Cia15
Copy link
Contributor Author

Cia15 commented Sep 13, 2023 via email

@@ -35,7 +35,7 @@ cpath = _setup_repo(false) # clean
@test !endswith(gitdescribe(cpath), "-dirty")

# tag!
function _test_tag!(d, path, haspatch, DRWATSON_STOREPATCH)
function _test_tag!(d, path, haspatch, has_message, DRWATSON_STOREPATCH)
Copy link
Member

Choose a reason for hiding this comment

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

Its great that you modified this function to test for the existence of a message. However I see two problems: 1) you never call this function with true as the 4th argument, and hence, the function never actually tests the new code. 2) the function does not test whether the message itself is correct. Only tests that it is not the empty string.

I propose for you to revert the modification of the test_tag! function. Instead, in the codeblock @testset "message_name" begin that you have created below, write a custom, hand coded test that explicitly calls tag! and then it explicitly tests that the tagged dictionary has exact information of the commit message, whatever that message is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thank you! I will try to implement it

Copy link
Member

Choose a reason for hiding this comment

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

The settings.json file still exists.

"""
gitpatch(gitpath = projectdir())

"""
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change happen? You should revert it.

@@ -128,6 +126,7 @@ function gitpatch(path = projectdir(); try_submodule_diff=true)
repo = LibGit2.GitRepoExt(path)
gitpath = LibGit2.path(repo)
gitdir = joinpath(gitpath,".git")
###gitmessage = LibGit2.message() #Add the gitmessage
Copy link
Member

Choose a reason for hiding this comment

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

This comment does not help, rather, it confuses. You should delete it.

src/saving_tools.jl Show resolved Hide resolved
@@ -208,6 +209,7 @@ function tag!(d::AbstractDict{K,T};
# Get the appropriate keys
commitname = keyname(d, :gitcommit)
patchname = keyname(d, :gitpatch)
message_name = keyname(d, :gitmessage) ### added
Copy link
Member

Choose a reason for hiding this comment

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

What information does the ### added comment provide? Does it really help? Remove the comment.

msg = LibGit2.message(mssgcommit)
if (msg !== nothing) && (msg != "")
d[message_name] = msg
end ### added
Copy link
Member

Choose a reason for hiding this comment

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

again, remove ### added.

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

I am sorry it took so long to submit another review. I was overwhelemd with other projects.

I am also sorry to drag this PR more, but the testing is still incorrect and needs to change. I am positive that this will be the last round of review though, I tried to make things explicit!

"""
gitpatch(gitpath = projectdir())
"""
gitpatch(gitpath = projectdir())
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect. Restore the 4 spaces of identation, otherwise the documentation string will not register correctly.

Comment on lines 171 to 172
stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the commit message is set to true,
then the output will display the commit message.
Copy link
Member

Choose a reason for hiding this comment

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

When referring to code, don't use english words. Use code, formatted as monotype font. Don't write "If the commit message is ...". Write instead "If the commit_message keyword is set to true, then...".

@@ -190,16 +192,18 @@ Dict{Symbol,Int64} with 2 entries:
:y => 4
:x => 3

julia> tag!(d)
julia> tag!(d, commit_message=true)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
julia> tag!(d, commit_message=true)
julia> tag!(d; commit_message=true)

it is better syntax to separate keywords by ;.

Comment on lines 81 to 82
# Ensure that the correct git message show up
d_new = tag!(d, gitpath=dpath, commit_message = true)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't test anything, see above.

Comment on lines 69 to 77
@testset "message_name" begin
path = mktempdir(cleanup=true) # delete path on process exit
repo = LibGit2.init(path)
LibGit2.commit(repo, "tmp repo commit")
message_commit_test = LibGit2.GitCommit(repo, "HEAD")
message_name = LibGit2.message(message_commit_test)
if (message_name !== nothing) && (message_name != "")
print(message_name)
end
Copy link
Member

Choose a reason for hiding this comment

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

This test block is wrong unfortunately. You need to test the tag! function. This is what DrWatson provides. We don't care about testing Lib2Git.

Please call the tag! function, with the keyword commit_message to true. Then manually examine the tagged dictionary, and manually write the test

@test d["gitmessage"] == "the mesage you expecet"

Not calling the @test macro means you are not testing anything. Printing is not testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this might be a dumb question, but will it also work if instead I add the @test d["gitmessage"] == "the mesage you expect" in the _test_tag! function? So something like this? I'm terribly sorry for the beginner mistakes..

image

Copy link
Member

Choose a reason for hiding this comment

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

Don't alter any of the previous code. It is simply not worth it. Why not just add this new test code block, that just has 5 lines of code, and does the test? Simpler = better.

(Sure, there could be ways to alter the previous code, but why do it though? just make a new @testset codeblock with 4 lines of code that call the tag! function and read the message and call an @test on the read message.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow it didn't work for me.............

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

Besides the outlined changes, you need to also:

  • Got to Project.toml and increment the minor (2nd number) version.
  • Got to CHANGELOG.md and add a new section under the new minor version where you state that you added this new functionality.

@@ -167,7 +168,8 @@ the project's gitpath). Do nothing if a key `gitcommit` already exists
repository is not found. If the git repository is dirty, i.e. there
are un-commited changes, and `storepatch` is true, then the output of `git diff HEAD` is stored
in the field `gitpatch`. Note that patches for binary files are not
stored. You can use [`isdirty`](@ref) to check if a repo is dirty.
stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the `commit message` is set to `true`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the `commit message` is set to `true`,
stored. You can use [`isdirty`](@ref) to check if a repo is dirty.
If the keyword `commit_message` is set to `true`,

it is important to be precise in documentation strings.

@@ -167,7 +168,8 @@ the project's gitpath). Do nothing if a key `gitcommit` already exists
repository is not found. If the git repository is dirty, i.e. there
are un-commited changes, and `storepatch` is true, then the output of `git diff HEAD` is stored
in the field `gitpatch`. Note that patches for binary files are not
stored. You can use [`isdirty`](@ref) to check if a repo is dirty.
stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the `commit message` is set to `true`,
then the output will display the commit message.
Copy link
Member

Choose a reason for hiding this comment

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

The sentence "then the output will display the commit message." should be imporved. What is correct is that "the dinctionary d will include an additional field "gitmessage" and will contain the git message associated with the commit.

test/stools_tests.jl Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Datseris Datseris merged commit cad2a38 into JuliaDynamics:main Sep 28, 2023
4 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