gap-packages / wedderga

Wedderburn Decomposition of Group Algebras
https://gap-packages.github.io/wedderga/
GNU General Public License v2.0
3 stars 6 forks source link

Fix fi in GSOCA #61

Closed angeldelriomateos closed 4 years ago

angeldelriomateos commented 4 years ago

Fixing a wrong placement of one fi in the code of GlobalSplittingOfCyclotomicAlgebra

angeldelriomateos commented 4 years ago

Alex, could you verify that I did properly the pull request fixing fi?

olexandr-konovalov commented 4 years ago

@angeldelriomateos great, congratulations with submitting your 1st pull request!

There was one thing in the instruction: "Pay attention to the base branch (i.e. the branch which you suggest to modify) - for example, for PR #58 you need to select divalg instead of master." which escaped attention. As a result, instead of one commit you are trying to merge 9 commits. But no worries - you can luckily update that easily now, following instructions at https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-base-branch-of-a-pull-request - please do that.

olexandr-konovalov commented 4 years ago

@angeldelriomateos I just discovered that I can also change the base branch myself - learned something new today:) But I am not touching it so far until I will hear from you.

codecov[bot] commented 4 years ago

Codecov Report

Merging #61 into divalg will increase coverage by 0.50%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           divalg      #61      +/-   ##
==========================================
+ Coverage   74.79%   75.30%   +0.50%     
==========================================
  Files          11       11              
  Lines        5309     5313       +4     
==========================================
+ Hits         3971     4001      +30     
+ Misses       1338     1312      -26     
Impacted Files Coverage Δ
lib/div-alg.gi 68.53% <100.00%> (+1.46%) :arrow_up:
lib/crossed.gi 73.91% <0.00%> (+0.14%) :arrow_up:
angeldelriomateos commented 4 years ago

@angeldelriomateos great, congratulations with submitting your 1st pull request!

There was one thing in the instruction: "Pay attention to the base branch (i.e. the branch which you suggest to modify) - for example, for PR #58 you need to select divalg instead of master." which escaped attention. As a result, instead of one commit you are trying to merge 9 commits. But no worries - you can luckily update that easily now, following instructions at https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-base-branch-of-a-pull-request - please do that.

Done!

olexandr-konovalov commented 4 years ago

Perfecto! let's wait till the next round of tests pass.

olexandr-konovalov commented 4 years ago

Cherry-picked onto divalg in d98ea0140d25649cedf5170edac6acbce075e50b.

I used this to change commit message from

Fixing an wrong placement of fi in GloSplitOfCycAlg

to

Fix wrong placement of fi in GlobalSplittingOfCyclotomicAlgebra

"Fix" is shorted than fixing. Also, it's a common practice to write commit messages not to continue the phrase "This commit is doing...", but to continue "If applied, this commit will..."

Finally, you can search within commit messages, so GloSplitOfCycAlg will not be caught if you will e.g. ask for all commits mentioning GlobalSplittingOfCyclotomicAlgebra, so it's good to be consistent. If you use

olexandr-konovalov commented 4 years ago

Closed in d98ea0140d25649cedf5170edac6acbce075e50b