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

Remove some deprecated code in ./src/vs/base #103454

Closed
LeuisKen opened this issue Jul 28, 2020 · 27 comments
Closed

Remove some deprecated code in ./src/vs/base #103454

LeuisKen opened this issue Jul 28, 2020 · 27 comments
Assignees
Labels
debt Code quality issues
Milestone

Comments

@LeuisKen
Copy link
Contributor

LeuisKen commented Jul 28, 2020

I find that following methods are deprecated:

7 results - 2 files

src/vs/base/browser/dom.ts:
  74  };
  75  
  76: /** @deprecated ES6 - use classList*/
  77  export function hasClass(node: HTMLElement | SVGElement, className: string): boolean { return _classList.hasClass(node, className); }
  78: /** @deprecated ES6 - use classList*/
  79  export function addClass(node: HTMLElement | SVGElement, className: string): void { return _classList.addClass(node, className); }
  80: /** @deprecated ES6 - use classList*/
  81  export function addClasses(node: HTMLElement | SVGElement, ...classNames: string[]): void { return _classList.addClasses(node, ...classNames); }
  82: /** @deprecated ES6 - use classList*/
  83  export function removeClass(node: HTMLElement | SVGElement, className: string): void { return _classList.removeClass(node, className); }
  84: /** @deprecated ES6 - use classList*/
  85  export function removeClasses(node: HTMLElement | SVGElement, ...classNames: string[]): void { return _classList.removeClasses(node, ...classNames); }
  86: /** @deprecated ES6 - use classList*/
  87  export function toggleClass(node: HTMLElement | SVGElement, className: string, shouldHaveIt?: boolean): void { return _classList.toggleClass(node, className, shouldHaveIt); }
  88  

src/vs/base/common/strings.ts:
  15  
  16  /**
  17:  * @deprecated ES6: use `String.padStart`
  18   */
  19  export function pad(n: number, l: number, char: string = '0'): string {

I'd like to send a pull request to remove these deprecated methods and refactor all usages if you want.

@rebornix
Copy link
Member

Starting from @mjbvz , not sure where the deprecation information are from.

@jrieken
Copy link
Member

jrieken commented Aug 10, 2020

I'd like to send a pull request to remove these deprecated methods

👎 We do not accept pull requests like that

@jrieken jrieken added the debt Code quality issues label Aug 10, 2020
@jrieken
Copy link
Member

jrieken commented Aug 10, 2020

This is driven from .vscode/searches/es6.code-search and each and every user of the listed functions needs to become active.

@jrieken jrieken assigned joaomoreno and unassigned jrieken Aug 11, 2020
@jrieken
Copy link
Member

jrieken commented Aug 11, 2020

The classList usage is usually a big change, the others should be small... Starting with @joaomoreno, please pass it on to the next person

@joaomoreno
Copy link
Member

@jrieken I am out this month... sorry.

@joaomoreno joaomoreno assigned jrieken and unassigned joaomoreno Aug 11, 2020
@jrieken jrieken assigned isidorn and sandy081 and unassigned jrieken Aug 12, 2020
sandy081 added a commit that referenced this issue Sep 7, 2020
@sandy081 sandy081 added this to the September 2020 milestone Sep 8, 2020
sandy081 added a commit that referenced this issue Sep 8, 2020
sandy081 added a commit that referenced this issue Sep 8, 2020
@sandy081 sandy081 removed their assignment Sep 8, 2020
mjbvz added a commit that referenced this issue Sep 10, 2020
For #103454

This should be a direct map to the `.findIndex` mathod
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 10, 2020

Remaining:

src/vs/base/common/strings.ts

  • pad
  • startsWith
  • endsWith
  • repeat

src/vs/base/common/objects.ts

  •  assign

src/vs/base/browser/dom.ts

  • removeNode
  • hasClass and friends

src/vs/base/common/arrays.ts

  • first
  • find

mjbvz added a commit that referenced this issue Sep 10, 2020
isidorn added a commit that referenced this issue Sep 11, 2020
@isidorn
Copy link
Contributor

isidorn commented Sep 11, 2020

I have replaced my usages of the deprecated code. I believe I have cought all places, however if some remain feel free to ping me.
Assigning this to @joaomoreno since I saw a lot of usages in our widgets, though feel free to pass it on to the next person.

@isidorn isidorn assigned joaomoreno and unassigned isidorn Sep 11, 2020
@joaomoreno
Copy link
Member

joaomoreno commented Sep 14, 2020

Here's a useful find/replace input pair, for text based refactoring:

(add|remove|toggle)Class\(([^,]+), 
$2.classList.$1(

@joaomoreno
Copy link
Member

The unfortunate choice of having multiple class names on a single string Codicon.classNames: string and having DOM.addClass support that is the hardest problem here. Going with simple refactoring, powered by TypeScript, you immediately hit very bad things:

image

@mjbvz
Copy link
Collaborator

mjbvz commented Oct 20, 2020

Remaining (all under dom.ts):

@joaomoreno Assigning you to start. Please reassign to correct owners for this

@mjbvz mjbvz assigned joaomoreno and unassigned RMacfarlane and mjbvz Oct 20, 2020
@joaomoreno
Copy link
Member

@alexdima alexdima removed their assignment Oct 20, 2020
mjbvz added a commit that referenced this issue Oct 20, 2020
@mjbvz mjbvz removed their assignment Oct 20, 2020
@aeschli aeschli removed their assignment Oct 26, 2020
@Tyriar Tyriar closed this as completed in cece809 Oct 27, 2020
@jrieken jrieken reopened this Oct 27, 2020
@jrieken
Copy link
Member

jrieken commented Oct 27, 2020

oh, wow actually done

@jrieken
Copy link
Member

jrieken commented Oct 27, 2020

👏 and thanks everyone involved here

jrieken added a commit that referenced this issue Oct 27, 2020
@Tyriar Tyriar assigned jrieken and unassigned Tyriar Oct 28, 2020
@jrieken jrieken closed this as completed Oct 28, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

No branches or pull requests

17 participants