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

ENHANCE: CharacteristicSubgroupsLib to CharacteristicSubgroups #978

Merged
merged 2 commits into from
Dec 7, 2016

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Dec 4, 2016

This PR renames CharacteristicSubgroupsLib to CharacteristicSubgroups (as suggested in #975).
As previously discussed in #878, it avoids problems with the declaration in CRISP by removing the ``multiple declarations match'' error, if both declarations are the same.

The actual functionality provided (if CRISP is not loaded) is pathetic and not worth mentioning in the release announcement.


Copied by @alex-konovalov from commit message: "Also remove multi-declaration warning if both declarations are equal -- this allows the library to duplicate declarations from packages"

@codecov-io
Copy link

codecov-io commented Dec 4, 2016

Current coverage is 49.61% (diff: 95.65%)

Merging #978 into master will decrease coverage by 0.01%

@@             master       #978   diff @@
==========================================
  Files           424        424          
  Lines        222957     222959     +2   
  Methods        3430       3430          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         110661     110629    -32   
- Misses       112296     112330    +34   
  Partials          0          0          

Powered by Codecov. Last update 8efd681...334d22a

@olexandr-konovalov
Copy link
Member

I've also copied "Also remove multi-declaration warning if both declarations are equal -- this allows the library to duplicate declarations from packages" from the commit message to the comment above - IMHO its important change so it should be more visible there.

@fingolfin
Copy link
Member

fingolfin commented Dec 5, 2016

This looks sensible.

However, these are really two different changes which are bunched together, and one affects all of GAP by modifying how declarations work -- so I'd consider this the by far bigger change. Yet it is not mentioned in the headline of the commit message.

Therefore I'd greatly prefer if the commits could be split into two (I think this it is in general a good idea to keep independent changes in separate commits). This can be done in under two minutes like this:

git checkout branch-with-this-work   # here I assume you have a clean work tree...
git reset HEAD^    # undo the last commit, but leave the changes in work tree
git add -p    # use interactive add to only stage the changes for first commit
git commit    # make first commit
git add -p    # stage remaining changes
git commit

Of course, since the two changes split along file borders nicely, one can also simply explicitly "git add FILES" instead of an interactive add.

@hulpke
Copy link
Contributor Author

hulpke commented Dec 5, 2016

@fingolfin
Just to make sure I understand. You are proposing still one branch, just two commits.

@hulpke hulpke force-pushed the charsub branch 2 times, most recently from c95f52e to 334d22a Compare December 5, 2016 16:02
@fingolfin
Copy link
Member

@hulpke Yes, exactly as you have done it. Thank you very much!

@fingolfin fingolfin merged commit 9a36e8c into gap-system:master Dec 7, 2016
@hulpke hulpke deleted the charsub branch December 19, 2017 14:37
@olexandr-konovalov olexandr-konovalov added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants