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

Avoid using objects to implement Map #627

Merged
merged 1 commit into from Jan 23, 2019

Conversation

Projects
None yet
2 participants
@mlasson
Copy link
Contributor

mlasson commented Jan 14, 2019

This commit occurring with branch names which are parsable integers.

They were always shown first in the list of branches (even if they are
not starred) because the chrome implementation does not respect the
insertion order for keys that happen to be parseable integers.

It fixes Issue #626.

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes are based off of the develop branch
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses
@@ -85,7 +85,7 @@ export namespace Arrays {
value?: T;

// parent?: IHierarchicalItem<T>;
children: { [key: string]: IHierarchicalItem<T> } | undefined;
children: Map<string, IHierarchicalItem<T>> | undefined;

This comment has been minimized.

@mlasson

mlasson Jan 14, 2019

Author Contributor

I kept the "| undefined" here, but it seems to me that we could rewrite the code easily to use an empty map instead (or an empty object before). Same for descendants.
Is there a good reason ?

This comment has been minimized.

@eamodio

eamodio Jan 23, 2019

Owner

Mainly for performance, to avoid creating objects that will need to be GC'd that we won't need.

@mlasson mlasson force-pushed the mlasson:mlasson-do-no-implement-map-with-objects branch from e849c8d to 14931fe Jan 14, 2019

Fixes #626 - Avoid using object to implement Map
This commit fixes a bug occurring with branch names which are parsable
integers.

They were always shown first in the list of branches (even if they are
not starred) because the chrome implementation does not respect the
insertion order for keys that happen to be parseable integers.

@mlasson mlasson force-pushed the mlasson:mlasson-do-no-implement-map-with-objects branch from 14931fe to 65d33e5 Jan 14, 2019

@mlasson

This comment has been minimized.

Copy link
Contributor Author

mlasson commented Jan 14, 2019

Note that running "prettier" induce some modification on files that appear to be unrelated to this PR:

     modified:   CHANGELOG.md
     modified:   src/annotations/blameAnnotationProvider.ts
     modified:   src/commands/diffWithRevision.ts
     modified:   src/commands/openFileRevision.ts

Same for lint, that returns the following warning:

WARNING: C:/Users/mlasson/Documents/Git/vscode-gitlens/src/vsls/host.ts:78:9 - Promises must be handled appropriately

@eamodio eamodio merged commit e1c2c74 into eamodio:develop Jan 23, 2019

@eamodio

This comment has been minimized.

Copy link
Owner

eamodio commented Jan 23, 2019

@mlasson Awesome work! Thanks so much for your contribution! This will be in the next release!

@mlasson

This comment has been minimized.

Copy link
Contributor Author

mlasson commented Jan 23, 2019

@eamodio: Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment