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

Disable importSorter.sortOnBeforeSave #692

Merged
merged 5 commits into from
Feb 2, 2021

Conversation

paulius-valiunas
Copy link
Contributor

No description provided.

@kabentley
Copy link
Contributor

i don't think we should remove this. It is very helpful. Instead, remove sortOnBeforeSave

@paulius-valiunas
Copy link
Contributor Author

i don't think we should remove this. It is very helpful. Instead, remove sortOnBeforeSave

I've been thinking about how we use this extension and I'd like to replace as much of it as possible with eslint rules.

For this particular case, there is an eslint rule called @typescript-eslint/no-unused-vars that does the same thing.

If I disable sortOnBeforeSave, I'd still want to disable this one too just to reduce duplicate functionality between this extension and eslint.

I'm not a fan of disabling sort on save, I think most people will just forget this extension exists. It's like having an eslint rule set to "warn" but even less noticeable since it doesn't show yellow squiggles. I initially wasn't sure what to think of this extension and interpreted it as a way to at least partly enforce neatly sorted imports in our code base, but if you have a different idea, I can disable it, that's your call.

@roluk
Copy link
Member

roluk commented Feb 1, 2021

Could we also remove the following?

"eslint.codeAction.disableRuleComment": {
  "location": "sameLine"
},

This seems to me more like a personal preference and in many situations, I think having disable rule comments on a separate line to be more advantageous. It follows our standard comment style, keeps code width within 120 columns and is much easier to remove (Ctrl+L, Backspace), which I do quite often to see if the disable comment is still required.

Those who wish to have it on same line can easily add this configuration to their user settings.

@paulius-valiunas
Copy link
Contributor Author

Could we also remove the following?

"eslint.codeAction.disableRuleComment": {
  "location": "sameLine"
},

This seems to me more like a personal preference and in many situations, I think having disable rule comments on a separate line to be more advantageous. It follows our standard comment style, keeps code width within 120 columns and is much easier to remove (Ctrl+L, Backspace), which I do quite often to see if the disable comment is still required.

Those who wish to have it on same line can easily add this configuration to their user settings.

I applied this as a fix to some other comments not being detected. I think it was either API extractor and our docs comments, or istanbul ignore comments. Something had to be directly above a line or one of them wouldn't be detected.

@paulius-valiunas
Copy link
Contributor Author

you know what, Robert, I think you're right on this one. For the rare case where it creates a conflict, you can always move the comment to the same line yourself.

@kabentley
Copy link
Contributor

i don't think we should remove this. It is very helpful. Instead, remove sortOnBeforeSave

I've been thinking about how we use this extension and I'd like to replace as much of it as possible with eslint rules.

For this particular case, there is an eslint rule called @typescript-eslint/no-unused-vars that does the same thing.

If I disable sortOnBeforeSave, I'd still want to disable this one too just to reduce duplicate functionality between this extension and eslint.

I'm not a fan of disabling sort on save, I think most people will just forget this extension exists. It's like having an eslint rule set to "warn" but even less noticeable since it doesn't show yellow squiggles. I initially wasn't sure what to think of this extension and interpreted it as a way to at least partly enforce neatly sorted imports in our code base, but if you have a different idea, I can disable it, that's your call.

Having sortOnBeforeSave eliminates the possibility that anyone could have a required order-of-imports dependency.

@paulius-valiunas
Copy link
Contributor Author

OK, point taken.
I'm still tempted to fully replace this extension with eslint rules and remove it in the future to reduce overlap. There's really not that much functionality missing even with our existing eslint config. Any thoughts on that?

@kabentley
Copy link
Contributor

OK, point taken.
I'm still tempted to fully replace this extension with eslint rules and remove it in the future to reduce overlap. There's really not that much functionality missing even with our existing eslint config. Any thoughts on that?

I'd far rather it be handled by eslint rather than the VSCode extension because not everyone (e.g. @pmconne) uses VSCode. Does the eslint rule no-unused-vars really remove unused entries from import statements?

@paulius-valiunas
Copy link
Contributor Author

I just checked that again, and it appears that it's not the eslint rule, but rather VSCode itself providing the fix through the ctrl+. shortcut (eslint suggestions appear in the same place, that's why I was confused). Here's an example:
image
If you're not using VSCode, eslint will just make you fix it yourself:

D:\code\iModelJs\imodeljs\core\backend\src\IModelHost.ts(11,13): error @typescript-eslint/no-unused-vars : 'semver' is defined but never used. Allowed unused vars must match /^_/u.
D:\code\iModelJs\imodeljs\core\backend\src\IModelHost.ts(16,74): error @typescript-eslint/no-unused-vars : 'RpcConfiguration' is defined but never used. Allowed unused vars must match /^_/u.
D:\code\iModelJs\imodeljs\core\backend\src\IModelHost.ts(16,92): error @typescript-eslint/no-unused-vars : 'SerializedRpcRequest' is defined but never used. Allowed unused vars must match /^_/u.
D:\code\iModelJs\imodeljs\core\backend\src\IModelHost.ts(18,44): error @typescript-eslint/no-unused-vars : 'AuthorizedClientRequestContext' is defined but never used. Allowed unused vars must match /^_/u.D:\code\iModelJs\imodeljs\core\backend\src\IModelHost.ts(18,96): error @typescript-eslint/no-unused-vars : 'UserInfo' is defined but never used. Allowed unused vars must match /^_/u.
D:\code\iModelJs\imodeljs\core\backend\src\IModelHost.ts(30,10): error @typescript-eslint/no-unused-vars : 'IModelJsFs' is defined but never used. Allowed unused vars must match /^_/u.

Either way, it still does the same as the extension, i.e. fixes the issue only if you're using VSCode.

It wouldn't be too hard to make the rule auto-fixable so that VSCode isn't required for this, but I'd have to either "fork" the rule (or copy/paste the code into our repository) or convince someone at @typescript-eslint to merge it into their master.

@kabentley
Copy link
Contributor

It wouldn't be too hard to make the rule auto-fixable so that VSCode isn't required for this, but I'd have to either "fork" the rule (or copy/paste the code into our repository) or convince someone at @typescript-eslint to merge it into their master.

Why not just leave the "remove unused" rule on for the extension? What prompted this PR? I presume the problem was that it was removing some imports from .tsx files (that's what i saw)? If we don't force it to run on those files every time save, then it isn't a problem.

BTW, does the eslint rule complain about unsorted imports?

@pmconne
Copy link
Member

pmconne commented Feb 1, 2021

BTW, does the eslint rule complain about unsorted imports?

Yes, the linter will complain about those.

@paulius-valiunas
Copy link
Contributor Author

Yeah, I got a complaint about tsx files, especially if you're using autosave. If the extension doesn't run on save, leaving it on doesn't really hurt. I thought eslint did the same thing so I wanted to disable it just to reduce duplication, but since I discovered it's a separate VSCode action, I guess it makes sense to add it back to reduce the number of separate actions you have to take to fix your imports.

When it comes to sorting imports in eslint, it's a bit more complicated since there are different rules for:

  • Sorting individual imports, such as swapping BackendIpc and SerializedRpcRequest in the above screenshot if they're misplaced. We have an eslint rule for that part, which by the way could cause conflicts with this extension due to different handling of aliases.
  • Sorting entire import statements, such as swapping the imodelhub-client line with the imodeljs-common line. There is an existing eslint rule for that, but we're not using it. Back when I had it enabled in a draft PR, it was the slowest rule of all by a wide margin. I then decided I'd write my own because it doesn't have to be that complicated, but it's still not done since it's not at the highest priority.

The import sorter extension covers both of these, which is pretty much the only reason why I'm still keeping it.

@paulius-valiunas paulius-valiunas changed the title Disable importSorter.removeUnusedImports Disable importSorter.sortOnBeforeSave Feb 1, 2021
@paulius-valiunas paulius-valiunas marked this pull request as ready for review February 2, 2021 09:30
@paulius-valiunas paulius-valiunas requested a review from a team as a code owner February 2, 2021 09:30
@paulius-valiunas paulius-valiunas enabled auto-merge (squash) February 2, 2021 09:32
@paulius-valiunas paulius-valiunas merged commit 931c320 into master Feb 2, 2021
@paulius-valiunas paulius-valiunas deleted the import-sorter-remove-unused branch February 2, 2021 20:11
kabentley added a commit that referenced this pull request Feb 11, 2021
* iModelHub release tags (#644)

* Change iModelHub api release tags

* extract-api fixes

* extract-api changes

* Change file

* PR fixes

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Update icon handling to workaround MacOS Electron crash (#642)

An invalid, or undefined, icon passed to the BrowserWindow causes a crash on MacOS with Electron 11, related Electron issue, electron/electron#27303.

* 2.12.0-dev.17

* Add IModelTransformOptions.isReverseSynchronization to better support synchronizing changes from a branch back to master (#671)

* Add IModelTransformOptions.isReverseSynchronization to better support synchronizing changes from a branch back to master.

* 2.12.0-dev.18

* Advertise change to default behavior of IModelDb.close() (#673)

* change to default behavior of IModelDb.close() and IModelDb.saveChanges()

Co-authored-by: Affan Khan <khanaffan@users.noreply.github.com>

* 2.11.0 changelogs (#677)

Co-authored-by: imodeljs-admin <38288322+imodeljs-admin@users.noreply.github.com>

* Mobile ipc take2 (#659)

* Initalize mobile BackendIpc before IModelHost.startup needs it

* change

* Do not rely on transport in constructor -- unnecessary entanglement with RPC world

* api

* missing event in handler call

* test fix and uv event loop fix

* lint

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* AccuDraw bi-directional value updates (#674)

* Improved AccuDraw input field, formatting using KOQs, updating value on mouse motion

* Unit tests. Added FrameworkAccuDrawUiAdmin for hasInputFocus.

* extract-api

* Support for grabInputFocus

* Minor fixes before merging in Raman's briefcase & Electron fixes

* Fixed selecting values to UI

* Unit test

* Fixed value from UI on delay

* Better AccuDraw input field focus & selection handling

* rush change

* Vertical/Horizontal layouts, multiple AccuDraw dialogs open, more rotate shortcuts, UiFramework.EscapeToHome setting

* rush change

* Changed ToolAdmin.onKeyTransition so onCtrlKeyPressed and processShortcutKey can be overridden

* extract-api, unit tests, removed onKeyTransition from tools

* Cleaned up isContextMenuOpen

* Removed unused value handling from AccuDrawFieldContainer

* Initializing value in AccuDrawInputField

* Fixed KeyShortcutMenu keydown/keyup issue. Rearranged AccuDraw shortcuts - Rotation commands now under 'A' key.

* extract-api

* Fixed AccuDrawInputField unit test requestNextAnimation problem

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* 2.12.0-dev.19

* Preceding checkpointv2 relationship fix (#611)

* Changed the relationship name used for preceding Checkpointv2 query

* API break: changed the CheckpointV2Query.precedingCheckpoint method name to CheckpointV2Query.precedingCheckpointV2

* 2.12.0-dev.20

* Allow target _blank in message boxes (#651)

* changes to allow target=_blank if secure relationship is present

* add tests

* changes

* fix linter errors

* fix coverage

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* 2.12.0-dev.21

* UI: Fix calling `scrollToNode` too early triggering an assertion error (#689)

If `scrollToNode` is called within `useEffect` that is run immediately after `TreeRenderer` component is mounted, an assertion error occurs. This is due to contents of `TreeRenderer` being rendered in two passes: first it determines its container width and height, and then it mounts `VariableSizeList` on the second pass.

This change introduces a new internal component called `VirtualizedList` which wraps `VariableSizeList` and adds the expected behaviour regarding `scrollToNode`. I don't think this is the ideal solution, the better one is to move `AutoSizer` component outside and let `TreeRenderer` consumers to specify tree width and height, but that would be a breaking change.

* Correctly handle capitalized SyncEventIds (#693)

* Correctly handle capitalized SyncUiEventsIds.

* Rush change.

* Fix bad merge with NextVersion.md (#695)

* Fix regression tests (#686)

* try increasing heap size

* lower ui-components coverage threshold by .1%

* Search for imodeljs-native in externals plugin in @bentley/imodeljs-backend and appNodeModules (#672)

* Search for imodeljs-native in imodeljs-backend and not just appNodeModules

* rush change

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Additional work to support case insensitive SyncEventId checking (#696)

* Update remaining syncEventId checks to be case insensitive.

* Update markdown docs formatting (#572)

* fix markdown lint errors
* Apply markdownlint automated formatting
* Consistently use `-` for lists instead of `*`.

* 2.12.0-dev.22

* Disable importSorter.sortOnBeforeSave (#692)

* Disable importSorter.removeUnusedImports

* move disable comments to above line by default

* disable import sorter action on save

* re-enable removeUnusedImports

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* 2.12.0-dev.23

* 2.13.0-dev.0

* 2.13.0-dev.1

* Fix 64-bit addition. (#703)

* Futureon briefcase (#658)

allow briefcaseId to be provided to bridge runner
Co-authored-by: admccarthy1 <admccarthy1@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* changing usage of bridges to connectors (#701)


Co-authored-by: Paul Connelly <22944042+pmconne@users.noreply.github.com>

* Conditionally disable/hide keyboard shortcuts (#704)

* Conditionally disable/hide keyboard shortcuts

* Rush change

* Fix the floating setTimeout in introspection client (#655)

* Fix the floating setTimeout
* Add basic set of tests for ResponseCache

* Azcopy for upload (#288)

* Added azcopy support for file upload
* Minor function parameter name refactoring

* 2.13.0-dev.2

* Clarify/expand FeatureOverrides documentation. (#708)

* Presentation: Update ModelsTree rulesets to better handle update (#709)

* Update ModelsTree rulesets

* rush change

* Fix maxWidth of iModelJs about dialog to not use clamp CSS function (#705)

* Removed use of CSS clamp function as it's not supported on all browsers. Also removed unnecessary calc calls from the CSS.

* rush change

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Enforce limits on tile GPU memory usage (#688)

* OrderedSet; DisclosedTileTreeSet.

* Do not override Tile.collectStatistics - that's what _collectStatistics is for.

* WIP track tile memory by category.

* TileMemoryBreakdown

* Memory breakdown UI.

* docs etc

* Test proving we currently reload parent tile content when children are selectable (fails).

* If parent tile's content is loaded and then later unloaded to converse memory, don't reload it if children are selectable.

* MemoryTracker can display stats for selected tiles.

* WIP track tile memory usage.

* ViewportIdSet

* Test ViewportIdSet.

* WIP test LRUTileList.

* Fix node insertion/removal.

* More tests.

* Fix clearSelectedForViewport; tests.

* docs clarification

* docs

* Remove redundant and now-unwieldy TileAdmin abstract base class.

* TileAdmin has a LRUTileList.

* Hook up memory limits in TileAdmin.

* typo

* wip more tests.

* More tests.

* fix merge.

* `fdt tile max mem` key-in

* fix key-in docs.

* Do not discard contents of tiles we want to draw but can't yet.

* Iterators over LRUTileList partitions.

* Additional info in TileMemoryBreakdown.

* Map tiles opt-out of memory limits.

* TileAdmin docs; GpuMemoryLimit(s)

* doc.

* GpuMemoryLimits replace mobileExpirationMemoryThreshold and TileTree.forcePrune.

* Add tests and fix bugs revealed thereby.

* More tests => more bug fixes.

* Adjust key-in for GpuMemoryLimit.

* docs.

* lint

* extract-api

* NextVersion.md

* broken doc link

* fix outdated tool Id

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* AccuDraw notifications for rotation & compass mode (#714)

* Added AccuDraw notifications for rotation & compass mode

* rush change

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* 2.13.0-dev.3

* Add a Quick Start page to the Learning/UI docs (#715)

* Getting Started UI

* Fix doc links, add npm install to web viewer instructions

* Documentation tweaks.

* More Tweaks

* Correct spelling

* Back out npm install

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Volume classification in dpta (#722)

* fix for running volume classification in DPTA

* rush change

* 2.13.0-dev.4

* 2.13.0-dev.5

* 2.13.0-dev.6

* Fix image diff regressions (#725)

* display-performance-test-app logs selected tile Ids.

* Fix image diff regressions.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* AccuDraw default keyboard shortcuts (#726)

* AccuDraw default keyboard shortcuts

* rush change

* Clean up

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Update UI docs (#723)

* Fix spelling errors in links

* Fix link errors.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Add support for common table expression (#697)

* cte test and docs

Co-authored-by: Affan Khan <khanaffan@users.noreply.github.com>

* Add missing closing pren to SampleNavigationWidget (#728)

* 2.13.0-dev.7

* Nonlocatable classification (#732)

* #449573 - implement nonlocatable for classifiers

* rush change

* Correct bad test to limit texture size.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Added AccuDraw widget (#734)

* Added AccuDraw widget

* extract-api && rush change

* #538236 - Add basic parsing support for Accudraw fields (#727)

* #538236 - Add basic parsing support for accudraw fields
* Replace ParseResult with QuantityParserResult

* 2.13.0-dev.8

* UI/ModelsTree: Fix getting elements' display status (#737)

* Floating widget opacity for UI 2.0 (#743)

* Floating widget opacity for UI 2.0

* rush change

* Clean up

* Added ElementSetTool base class. (#738)

* Added ElementSetTool base class.

* Add doc comment for ElementAgenda.elements.

* Re-run extract-api

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Support for ArcGIS token based authentication. (#575)

* WIP ArcGis Token geneator and cache.

* Fixed issue whre info URL was incorrectly derived from base URL.

* Added MapLayerStatus, check server response for token errors, added layer status icon in the UI and allow credentials to be provided with re-attaching the layer.

* Attempt to renew the token using existing credentials (only once), otherwise change to status of the layer and wait for end-user to provide new credentials.
Removed debugging code.

* Revert "Attempt to renew the token using existing credentials (only once), otherwise change to status of the layer and wait for end-user to provide new credentials."

This reverts commit 44cb714.

* Readded previous commit without the fomatting change.

* Removed debugging code.

* Moved layer status outside MapLayerProps

* Updated UI to support arcgis token. Code cleaning / linting.

* Token support for tooltip request.

* Change the ArcGisTokenGenetor interface, credentials are no longer part of the option objects.  Previous token cache is cleared when user re-enter credentials. No longer store token in local storage.

* Add method to get the imagery provider for a map layer

* Added username/password to compareTileTreeIds

* UI provide better feedback when a token expire, crendentials need to be provided or credentials are invalid.

* Moved username/password from MapLayerProps to MapLayerSettings.

* Make sure ArcGIS layers can be saved in settings services (without credentials)

* Avoid duplicated error messages when a layer requiring credentials is attached.

* Added missing release tag.

* Added change logs.

* Added API extracts.

* Few adjustments based on PR comments.

* Removed invalid comment.

* Removed no longer needed import.

* Removed no longer needed 'InteractiveEditingSession' import.

* Added missing api change files.

Co-authored-by: Ray.Bentley <rbbentley@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* UI/AccuDraw 2d views (#746)

* AccuDraw show Z field only for 3d views

* rush change

* Consolidate configuration setup across repository (#685)

* Update use of configuration to consistently use .env file. Deprecate use of default.json.

- Add new custom Rush command, 'copy:config', to copy a shared `.env` file to each directory necessary
- Enhance the internal-tools copyConfig script to allow a custom path for `.env` file
- Mark main IModelJsConfig class as deprecated

* 2.13.0-dev.9

* lint

* extract-api

* merge

* lint errors

* documenation descriptions

* doc errors

* documentation cleanup

* revert ecschema-metadata changes

Co-authored-by: Karolis Dziedzelis <3297568+Animagne@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Caleb Shafer <31107829+calebmshafer@users.noreply.github.com>
Co-authored-by: imodeljs-admin <38288322+imodeljs-admin@users.noreply.github.com>
Co-authored-by: scsewall <36768600+scsewall@users.noreply.github.com>
Co-authored-by: affank <khanaffan@gmail.com>
Co-authored-by: Affan Khan <khanaffan@users.noreply.github.com>
Co-authored-by: swbsi <69857376+swbsi@users.noreply.github.com>
Co-authored-by: Dan East <51122937+DanEastBentley@users.noreply.github.com>
Co-authored-by: Austėja Kalpakovaitė <70565417+austeja-bentley@users.noreply.github.com>
Co-authored-by: kckst8 <6283674+kckst8@users.noreply.github.com>
Co-authored-by: Robert Lukasonok <70327485+roluk@users.noreply.github.com>
Co-authored-by: GerardasB <10091419+GerardasB@users.noreply.github.com>
Co-authored-by: nick4598 <22119573+nick4598@users.noreply.github.com>
Co-authored-by: bsteinbk <65047615+bsteinbk@users.noreply.github.com>
Co-authored-by: Paulius Valiunas <66480813+paulius-valiunas@users.noreply.github.com>
Co-authored-by: Paul Connelly <22944042+pmconne@users.noreply.github.com>
Co-authored-by: admccarthy1 <76180977+admccarthy1@users.noreply.github.com>
Co-authored-by: timlawrence-bentley <76966166+timlawrence-bentley@users.noreply.github.com>
Co-authored-by: Saulius Skliutas <24278440+saskliutas@users.noreply.github.com>
Co-authored-by: toddsouthenbentley <48103957+toddsouthenbentley@users.noreply.github.com>
Co-authored-by: NancyMcCallB <45079789+NancyMcCallB@users.noreply.github.com>
Co-authored-by: MarcNeely <36053767+MarcNeely@users.noreply.github.com>
Co-authored-by: Grigas <35135765+grigasp@users.noreply.github.com>
Co-authored-by: bbastings <65233531+bbastings@users.noreply.github.com>
Co-authored-by: Michel D'Astous <mdastous-bentley@users.noreply.github.com>
Co-authored-by: Ray.Bentley <rbbentley@users.noreply.github.com>
Co-authored-by: aurislt7 <30312645+aurislt7@users.noreply.github.com>
Co-authored-by: Robert Schili <rschili@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants