Refactor and improve Molecule API's SMILES endpoint for better maintainability and performance #2246
Conversation
…maintainability and performance
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
app/api/chemotion/molecule_api.rb
Outdated
| molecule = Molecule.find_or_create_dummy if molecule.blank? | ||
| end | ||
| return unless molecule | ||
| molecule = Chemotion::SmilesProcessor.new(params).process |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [244/200]
app/api/chemotion/molecule_api.rb
Outdated
| molecule = Molecule.find_or_create_dummy if molecule.blank? | ||
| end | ||
| return unless molecule | ||
| molecule = Chemotion::SmilesProcessor.new(params).process |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [242/200]
| if svg.present? | ||
| svg_process = SVG::Processor.new.structure_svg(params[:editor], svg, molfile) | ||
| mol = molecule.molfile.lines.first(2) | ||
| if mol[1]&.strip&.match?('OpenBabel') |
There was a problem hiding this comment.
Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.
lib/svg/processor.rb
Outdated
| end | ||
| end | ||
|
|
||
|
|
There was a problem hiding this comment.
Layout/EmptyLines: Extra blank line detected.
| iupac_name: 'cubane', | ||
| names: ['cubane'] | ||
| } | ||
| context 'with decoupled true' do |
There was a problem hiding this comment.
RSpec/NestedGroups: Maximum example group nesting exceeded [4/3].
| expect(m.molecule_svg_file).to match(/\w{128}\.svg/) | ||
| end | ||
|
|
||
| context 'with missing SVG file' do |
There was a problem hiding this comment.
RSpec/NestedGroups: Maximum example group nesting exceeded [4/3].
LCOV of commit
|
app/api/chemotion/molecule_api.rb
Outdated
| @@ -30,59 +30,10 @@ class MoleculeAPI < Grape::API | |||
| optional :editor, type: String, desc: 'SVGProcessor', default: 'ketcher' | |||
| end | |||
| post do | |||
| smiles = params[:smiles] | |||
| svg = params[:svg_file] | |||
| molecule, temp_svg, ob_log = Chemotion::SmilesProcessor.new(params).process | |||
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [242/200]
|
|
||
| it 'returns a molecule instance with valid attributes' do | ||
| processor_result = described_class.new(params).process | ||
| binding.pry |
There was a problem hiding this comment.
Lint/Debugger: Remove debugger entry point binding.pry.
LCOV of commit
|
| @@ -30,59 +30,11 @@ class MoleculeAPI < Grape::API | |||
| optional :editor, type: String, desc: 'SVGProcessor', default: 'ketcher' | |||
| end | |||
| post do | |||
| smiles = params[:smiles] | |||
| svg = params[:svg_file] | |||
| result = Chemotion::SmilesProcessor.new(params).process | |||
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [243/200]
LCOV of commit
|
| end | ||
|
|
||
| it 'has different inchikeys' do | ||
|
|
There was a problem hiding this comment.
Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body beginning.
| end | ||
|
|
||
| it 'creates a new bad molecule if it does not exist' do | ||
|
|
There was a problem hiding this comment.
Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body beginning.
|
|
||
| it 'creates a new bad molecule if it does not exist' do | ||
|
|
||
| expect(described_class.find_by(inchikey: bad_info[:inchikey], is_partial: false)) |
There was a problem hiding this comment.
RSpec/VoidExpect: Do not use expect() without .to or .not_to. Chain the methods or remove it.
| end | ||
|
|
||
| it 'does not create a good molecule after a bad one' do | ||
| molecule = described_class.find_or_create_by_molfile(bad_info[:molfile]) |
There was a problem hiding this comment.
Lint/UselessAssignment: Useless assignment to variable - molecule. Did you mean molecule_good?
| molecule_good = described_class.find_or_create_by_molfile(good_info[:molfile]) | ||
| expect(molecule_good).to be_persisted | ||
| expect(molecule_good.inchikey).to eq(good_info[:inchikey]) | ||
| end |
There was a problem hiding this comment.
RSpec/EmptyLineAfterExample: Add an empty line after it.
| expect(molecule_good.inchikey).to eq(good_info[:inchikey]) | ||
| end | ||
| it 'does not transform a bad smiles into molfile' do | ||
|
|
There was a problem hiding this comment.
Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body beginning.
| end | ||
| it 'does not transform a bad smiles into molfile' do | ||
|
|
||
| rw_mol = begin |
There was a problem hiding this comment.
Layout/TrailingWhitespace: Trailing whitespace detected.
|
|
||
| rw_mol = begin | ||
| RDKitChem::RWMol.mol_from_smiles(smiles_bad) | ||
| rescue StandardError => e |
There was a problem hiding this comment.
Layout/RescueEnsureAlignment: rescue at 105, 15 is not aligned with rw_mol = begin at 103, 6.
| rw_mol = begin | ||
| RDKitChem::RWMol.mol_from_smiles(smiles_bad) | ||
| rescue StandardError => e | ||
| e.message |
There was a problem hiding this comment.
Layout/IndentationWidth: Use 2 (not 3) spaces for indentation.
| RDKitChem::RWMol.mol_from_smiles(smiles_bad) | ||
| rescue StandardError => e | ||
| e.message | ||
| end |
There was a problem hiding this comment.
Layout/BeginEndAlignment: end at 107, 15 is not aligned with rw_mol = begin at 103, 6.
rather 1-story 1-commit than sub-atomic commits
commit title is meaningful => git history search
commit description is helpful => helps the reviewer to understand the changes
code is up-to-date with the latest developments of the target branch (rebased to it or whatever) => ⏩-merge for linear history is favoured
added code is linted
tests are passing (at least locally): we still have some random test failure on CI. thinking of asking spec/examples.txt to be commited
in case the changes are visible to the end-user, video or screenshots should be added to the PR => helps with user testing
testing coverage improvement is improved.
CHANGELOG : add a bullet point on top (optional: reference to github issue/PR )
parallele PR for documentation on docusaurus if the feature/fix is tagged for a release