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

fix function signatures #773

Closed
wants to merge 2 commits into from
Closed

Conversation

UziTech
Copy link
Collaborator

@UziTech UziTech commented Jan 18, 2021

Some of the functions that that were supposed to return arrays were not.

@UziTech UziTech requested a review from aminya January 18, 2021 23:16
@UziTech
Copy link
Collaborator Author

UziTech commented Jan 18, 2021

@aminya I'm not sure how to set the minimapEditor for the tests.

@@ -1,2 +1,2 @@
"use strict";Object.defineProperty(exports,"__esModule",{value:!0}),require("atom");const e=require("./main-3af7aad3.js");require("path"),exports.Minimap=e.Minimap,exports.activate=e.activate,exports.activatePlugin=e.activatePlugin,exports.config=e.config,exports.deactivate=e.deactivate,exports.deactivateAllPlugins=e.deactivateAllPlugins,exports.deactivatePlugin=e.deactivatePlugin,Object.defineProperty(exports,"domStylesReader",{enumerable:!0,get:()=>e.domStylesReader}),Object.defineProperty(exports,"editorsMinimaps",{enumerable:!0,get:()=>e.editorsMinimaps}),exports.emitter=e.emitter,exports.getActiveMinimap=e.getActiveMinimap,exports.getConfigSchema=e.getConfigSchema,exports.getPluginsOrder=e.getPluginsOrder,exports.minimapClass=e.minimapClass,exports.minimapForEditor=e.minimapForEditor,exports.minimapForEditorElement=e.minimapForEditorElement,exports.minimapViewProvider=e.minimapViewProvider,exports.observeMinimaps=e.observeMinimaps,exports.onDidActivate=e.onDidActivate,exports.onDidActivatePlugin=e.onDidActivatePlugin,exports.onDidAddPlugin=e.onDidAddPlugin,exports.onDidChangePluginOrder=e.onDidChangePluginOrder,exports.onDidCreateMinimap=e.onDidCreateMinimap,exports.onDidDeactivate=e.onDidDeactivate,exports.onDidDeactivatePlugin=e.onDidDeactivatePlugin,exports.onDidRemovePlugin=e.onDidRemovePlugin,exports.plugins=e.plugins,exports.provideMinimapServiceV1=e.provideMinimapServiceV1,exports.registerPlugin=e.registerPlugin,exports.standAloneMinimapForEditor=e.standAloneMinimapForEditor,exports.toggle=e.toggle,exports.togglePluginActivation=e.togglePluginActivation,exports.unregisterPlugin=e.unregisterPlugin;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not check in the built file. GitHub Action does it automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought they were added to .gitignore?

Copy link
Collaborator

@aminya aminya Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but build-commit does some git magic to add them to the repo. If you run the npm run build-commit from bash, this should not happen. I have not found a general solution yet.

atom-community/atom-ide-template#25

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, I thought npm run build-commit commits all of the files. You are telling me now that it prevents committing the dist folder?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gitignore prevents doing that, however, build-commit renames gitignore temporarily, commits the dist folder, and renames back the gitignore to what it was. Then it runs a git script to ignore the changes of dist folder.

https://github.com/aminya/build-commit/blob/07f9fbfdd69409864b1b15319ee31a1e74f01a3e/bin/build-commit#L60

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking just committing the dist folder would be easier. I don't see any downsides to it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build-commit works for now. Ideally, we should fix the issue in apm. In the previous community meeting, there were suggestions for using Github packages for publishing using apm.
atom/apm#498

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems unlikely to happen any time soon, or at all, since it would require adding every Atom package to GitHub packages. That would mean Atom would lose any package that is not currently maintained.

I guess we will just keep having these conversations every time I forget to use build-commit 😁

@aminya
Copy link
Collaborator

aminya commented Jan 18, 2021

@aminya I'm not sure how to set the minimapEditor for the tests.

There are already some tests for some of these functions. Just add the expected part there.

https://github.com/UziTech/minimap/blob/84edff53492f2ef78e82d87885ba3cc480224fa3/spec/minimap-element-spec.js#L2261

@UziTech UziTech closed this Jan 19, 2021
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

2 participants