analysis

Clone Tools
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
SequencesInfo which is an individual object is doable, but I feel like it would be just a wrapper around a Map. What additional functionality would SequencesInfo have over a traditional Map? I'm n...

SequencesInfo which is an individual object is doable, but I feel like it would be just a wrapper around a Map. What additional functionality would SequencesInfo have over a traditional Map?

I'm not very familiar with serialization in java, I know there are libraries that can serialize beans to json or xml, but what are your expectations in this case?

one class with a static factory method seems to be better than two classes and an external mutable object.

one class with a static factory method seems to be better than two classes and an external mutable object.

What's wrong with static classes? They are just like regular classes but within other class' namespace. That's non-static inner classes that are special. About factory pattern, I think it would be ...

What's wrong with static classes? They are just like regular classes but within other class' namespace. That's non-static inner classes that are special.
About factory pattern, I think it would be an overkill for a class that is only used by SeqsetUtils' methods.

yes, I see that, but do you really need a nested static class ? in line with my general comments re encapsulating the Map<String,SequenceInfo> further: perhaps it might make sense to simply apply a...

yes, I see that, but do you really need a nested static class ? in line with my general comments re encapsulating the Map<String,SequenceInfo> further: perhaps it might make sense to simply apply a factory pattern rather than have an internal bean that a generic map holds references to ?

(lets not get stuck on this though, the main thing is that the implementation works fine for now, though needs more work in order to save/restore sequenceInfo objects from Jalview project files)

Because it's instantiated in a static context e.g. uniquify and it does not need an instance of SeqsetUtils object to work.

Because it's instantiated in a static context e.g. uniquify and it does not need an instance of SeqsetUtils object to work.

we won't see this messages unless asserts are enabled. Cache.log.warn is perhaps better ?

we won't see this messages unless asserts are enabled. Cache.log.warn is perhaps better ?

it's a reasonable first pass, though stylistically all you've done is transformed a single concrete instance (Hashtable) to a more strongly typed verbose instance (Map<String,SequenceInfo>). Why no...

it's a reasonable first pass, though stylistically all you've done is transformed a single concrete instance (Hashtable) to a more strongly typed verbose instance (Map<String,SequenceInfo>). Why not go further and have a SequencesInfo object that records metadata for one or more sequences ?

There are also other requirements: SequenceInfo sets will need to be persisted for a web services job. You could expand JAL-3899 to incorporate this perhaps - JAL-1786 is the relevant epic for that.

why does this class need to be static ?

why does this class need to be static ?

this should probably be Cache.log.warn(..)

this should probably be Cache.log.warn(..)

JAL-3899 Refactor sequence de/uniquification.
JAL-3899 Refactor sequence de/uniquification.
JAL-3899 Refactor sequence de/uniquification.

  1. … 1 more file in changeset.
JAL-3691 automatic insertion of Locale.ROOT to toUpperCase() and toLowerCase() and added import java.util.Locale appropriately

  1. … 101 more files in changeset.
JAL-3759 fix compilation error for test

JAL-3765 fix test

JAL-3765 fix test

JAL-3765 simple test that findAll finds all matches for a pattern when it is present multiple times in a sequence

JAL-3748 reusable assert to trace specific issues with recovering the correct Sequence/Mapping (highlights points of failure for AlignmentUtilsTests with patch

JAL-3490 merged with 2.11.2 develop

  1. … 22 more files in changeset.
JAL-3490 match count independent of contiguous matches count

  1. … 8 more files in changeset.
JAL-3490 match count independent of contiguous matches count

  1. … 8 more files in changeset.
Ah it does run when a column selection is made in a split frame (and propagated to the other panel). Which seems to work! Maybe just needs a less clumsy AlignmentViewport.isColSelChanged() - this m...

Ah it does run when a column selection is made in a split frame (and propagated to the other panel). Which seems to work!
Maybe just needs a less clumsy AlignmentViewport.isColSelChanged() - this method seems to be trying to do two different things.

Suggesting renaming to PROPERTY_COLUMNS = "columns" as this is used to notify changes to column selection or visibility, not sequences

Suggesting renaming to PROPERTY_COLUMNS = "columns" as this is used to notify changes to column selection or visibility, not sequences

I think this code path rarely gets run (AlignmentViewport.notifySequence() rarely gets called). So what is it there for?

I think this code path rarely gets run (AlignmentViewport.notifySequence() rarely gets called).
So what is it there for?

Suggest renaming notifyAlignment() to notifyAlignmentChanged(), and notifySequence() to notifyColumnChange() as it it fired for changes to column (not sequence) selection or visibility.

Suggest renaming notifyAlignment() to notifyAlignmentChanged(), and notifySequence() to notifyColumnChange() as it it fired for changes to column (not sequence) selection or visibility.

Unclear what notifySequence() adds. As placed here, it is hardly ever reached.

Unclear what notifySequence() adds. As placed here, it is hardly ever reached.

Not sure how to review this (and the appletgui/js classes) without pointers to documentation and/or examples of how it is used.

Not sure how to review this (and the appletgui/js classes) without pointers to documentation and/or examples of how it is used.

Needs proper Javadoc

Needs proper Javadoc

JAL-3446 unused imports removed

  1. … 34 more files in changeset.
Files added to doc folder: are this intended as working documents, or publishable documentation? Bearing in mind that that are findable on the web and a user might reasonably expect them to provide...

Files added to doc folder: are this intended as working documents, or publishable documentation? Bearing in mind that that are findable on the web and a user might reasonably expect them to provide documentation.
e.g. http://source.jalview.org/gitweb/?p=jalview.git;a=blob;f=doc/JalviewJS-startupParams.md;h=9ff33529b2dbc2da34d2a573ccf5d042194fbb0d;hb=refs/heads/Jalview-JS/develop