Opened 6 years ago

Closed 6 years ago

#14673 closed enhancement (fixed)

turn crystalographic into crystallographic

Reported by: chapoton Owned by: sage-combinat
Priority: minor Milestone: sage-5.10
Component: combinatorics Keywords: spelling
Cc: sage-combinat Merged in: sage-5.10.rc1
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw, Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The word crystallographic should be spelled with two "l".

The spelling with one "l" is very rare.

For a comparison, try your favorite search engine.

This patch changes all the occurences of 'crystalographic" into "crystallographic"

Attachments (1)

trac_14673_crystallo-fc.patch (37.7 KB) - added by chapoton 6 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by chapoton

  • Status changed from new to needs_review

comment:2 Changed 6 years ago by nthiery

Hi Frédéric,

While I definitely see this as a worthwhile improvement, one will need to devise a plan for:

  • maintaining backward compatibility, with appropriate deprecation, for some time
  • making sure that this does not conflict with too many patches outthere

Cheers,

Nicolas

comment:3 Changed 6 years ago by chapoton

I have added some aliases with deprecation warnings for backward compatibility.

comment:4 Changed 6 years ago by tscrim

Nicolas,

I don't think this will break much, if anything, in the queue. I'll try this out later tonight and can do the review then too.

Best,
Travis

comment:5 Changed 6 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

This didn't really break anything (some trivial rebasing due to the extra import). There might be one or two additional rebased in the queue that are necessary, but I'm giving this a positive review since the aliases will keep it backwards compatible.

Best,
Travis

comment:6 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.10

comment:7 Changed 6 years ago by nthiery

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Nicolas M. Thiéry
  • Status changed from positive_review to needs_work
  • Work issues set to missing backward compatibility alias in CartanType.samples

Hi Frédéric!

Thanks for fixing this original typo of mine :-)

I went through the patch and am happy with it up to two details:

  • Unless I missed it, there lacks a backward compatibility alias for CartanType?.samples(crystalographic=True).
  • It would be good to add tests for the backward compatibility aliases.

Changed 6 years ago by chapoton

comment:8 Changed 6 years ago by chapoton

  • Status changed from needs_work to needs_review

done

comment:9 Changed 6 years ago by tscrim

  • Work issues missing backward compatibility alias in CartanType.samples deleted

Looks good to me. Nicolas?

comment:10 Changed 6 years ago by nthiery

  • Status changed from needs_review to positive_review

+1!

Thanks Frédéric!

comment:11 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.10.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.