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

[PR#5468] refactor: remove TypeScript non null assertions #5518

Conversation

aloisklink
Copy link
Member

📑 Summary

Note

This PR does not target the develop branch. Instead it targets:

I'm not a big fan of using TypeScript's non-null assertion operator (e.g. map.get(val)!).

Instead, I've tried to rewrite the code when possible, so it's not needed. E.g., instead of doing:

if (map.has(x)) {
  return map.get(x);
}

I've tried to do things that TypeScript can understand, e.g.

const val = map.get(x);
if (val !== undefined) {
  return val;
}

There's also some other misc improvements I found when reviewing #5468.

There should be no functional changes to the code, this is a refactoring only change.

📏 Design Decisions

Describe the way your implementation works or what design decisions you made if applicable.

📋 Tasks

Make sure you

Remove the non-null assertion operator when possible from PR
mermaid-js#5468
@github-actions github-actions bot added the Type: Other Not an enhancement or a bug label May 13, 2024
@aloisklink aloisklink mentioned this pull request May 13, 2024
4 tasks
Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 84 lines in your changes are missing coverage. Please review.

Project coverage is 5.72%. Comparing base (50c9ede) to head (730fa89).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           fix/maps   #5518   +/-   ##
========================================
  Coverage      5.72%   5.72%           
========================================
  Files           278     277    -1     
  Lines         42018   42012    -6     
  Branches        490     515   +25     
========================================
  Hits           2407    2407           
+ Misses        39611   39605    -6     
Flag Coverage Δ
unit 5.72% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/mermaid/src/diagrams/git/gitGraphAst.js 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/state/stateDb.js 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/pie/pieRenderer.ts 0.00% <0.00%> (ø)
packages/mermaid/src/mermaidAPI.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/class/classDb.ts 0.00% <0.00%> (ø)
...ges/mermaid/src/diagrams/class/classRenderer-v2.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/sankey/sankeyDB.ts 0.00% <0.00%> (ø)
...ckages/mermaid/src/diagrams/sequence/sequenceDb.ts 0.00% <0.00%> (ø)
...id/src/diagrams/requirement/requirementRenderer.js 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/block/blockDB.ts 0.00% <0.00%> (ø)
... and 1 more

... and 1 file with indirect coverage changes

@Yash-Singh1
Copy link
Member

Yeah, this is definitely a better way to interact with Maps 👍🏾 .

@Yash-Singh1 Yash-Singh1 merged commit 00eaebe into mermaid-js:fix/maps May 15, 2024
13 checks passed
@aloisklink aloisklink deleted the 5468/refactor/remove-TypeScript-non-null-assertion-when-possible branch May 15, 2024 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Other Not an enhancement or a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants