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

Various Revisions #8

Closed
wants to merge 14 commits into from
Closed

Various Revisions #8

wants to merge 14 commits into from

Conversation

confused-Techie
Copy link
Member

@confused-Techie confused-Techie commented Sep 26, 2022

This PR covers several points of concern for the repo.

While this is still in progress the already completed/intended to be complete areas are:

  • Decaffeinate everything, from specs to src.
    • A note on decaffeination, while it may not be the prettiest code, it was decaffeinated with the tools that automatically did it during building. Meaning it should be nearly the exact some code that has been shipped so far. Exempt in some key areas:
      • Obvious uses of var are being replaced with the appropriate let or const
      • Additionally any instances of internal Express Servers being used are being rewritten, more on that later.
      • Pointless returns are being removed as needed.
  • Updates some dependencies:
    • express 4.17.1 => 4.18.1
    • node-gyp 5.1.1 => 9.1.0
  • Express Server code is being rewritten with modern conventions and syntax, to be more accessible to those newer with the platform, so that their docs match what we have.
  • All Coffee related dependencies and scripts have been removed
  • Scripts have been modified to support Windows systems that contain a path in the name. This resolves PR Support Windows System with Space in Path #4, while ignored whatever formatting error that happened on that PR.

Some things that should still be high priority to complete even if not present in this PR.

  • We should get git-utils to rely on our own fork. Although our fork doesn't seem to contain the tagged version referenced in our dependency. And downgrading to 5.7.0 produces many errors, and will likely have to be revisited.
  • Ideally we can move away from custom implementations of Jasmine like we have here jasmine-focused, which relies on a specific commit of kevinsawicki/jasmine-node which itself is forked from mhevery/jasmine-node meanwhile jasmine supports node just fine at modern versions and could likely save us some headache of finding documentation. But unfortunately our current tests rely on some features that no longer or have never existed on normal jasmine, so those will have to get significant rewrites to work.

Otherwise this PR will remain a draft until work is done on properly decaffeinating and properly rewriting the Express code.

Feel free to add more here, especially around dependencies, as I'd like to get everything updated that we can without to many mysterious test failures, and switch over to all of our forks for dependencies as much as possible.

};
};

describe('apm uninstall', function() {

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace 'apm·uninstall',·function with "apm·uninstall",·function·

Suggested change
describe('apm uninstall', function() {
describe("apm uninstall", function () {

spyOnToken();
atomHome = temp.mkdirSync('apm-home-dir-');
process.env.ATOM_HOME = atomHome;
atomReposHome = temp.mkdirSync('apm-repos-home-dir-');

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace 'apm-repos-home-dir-' with "apm-repos-home-dir-"

Suggested change
atomReposHome = temp.mkdirSync('apm-repos-home-dir-');
atomReposHome = temp.mkdirSync("apm-repos-home-dir-");

return function(error) {
var templatePath;
if (error != null) {
return callback(error);

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Insert ··

Suggested change
return callback(error);
return callback(error);

var currentDir;
silenceOutput();
spyOnToken();
currentDir = temp.mkdirSync('apm-init-');

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace 'apm-init-' with "apm-init-"

Suggested change
currentDir = temp.mkdirSync('apm-init-');
currentDir = temp.mkdirSync("apm-init-");

userconfig: config.getUserConfigPath(),
globalconfig: config.getGlobalConfigPath()
};
return npm.load(npmOptions, function() {

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Insert ·

Suggested change
return npm.load(npmOptions, function() {
return npm.load(npmOptions, function () {

return expect(lines.join("\n")).not.toContain('.bin');
});
});
describe('enabling and disabling packages', function() {

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace 'enabling·and·disabling·packages',·function with "enabling·and·disabling·packages",·function·

Suggested change
describe('enabling and disabling packages', function() {
describe("enabling and disabling packages", function () {

app.get('/node/v10.20.1/node-v10.20.1.tar.gz', function(request, response) {
return response.sendFile(path.join(__dirname, 'fixtures', 'node-v10.20.1.tar.gz'));
});
app.get('/node/v10.20.1/node-v10.20.1-headers.tar.gz', function(request, response) {

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace '/node/v10.20.1/node-v10.20.1-headers.tar.gz',·function with ⏎········"/node/v10.20.1/node-v10.20.1-headers.tar.gz",⏎········function·

Suggested change
app.get('/node/v10.20.1/node-v10.20.1-headers.tar.gz', function(request, response) {
app.get(

});
server = http.createServer(app);
live = false;
server.listen(3000, '127.0.0.1', function() {

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace '127.0.0.1',·function with "127.0.0.1",·function·

Suggested change
server.listen(3000, '127.0.0.1', function() {
server.listen(3000, "127.0.0.1", function () {

return options.alias('d', 'dev').boolean('dev').describe('dev', 'Link to ~/.atom/dev/packages');
};

Link.prototype.run = function(options) {

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Insert ·

Suggested change
Link.prototype.run = function(options) {
Link.prototype.run = function (options) {

return;
}
try {
metadata = JSON.parse(fs.readFileSync(path.join(packageDirectory, 'package.json')));

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace fs.readFileSync(path.join(packageDirectory,·'package.json')) with ⏎··········fs.readFileSync(path.join(packageDirectory,·"package.json"))⏎········

Suggested change
metadata = JSON.parse(fs.readFileSync(path.join(packageDirectory, 'package.json')));
metadata = JSON.parse(

var configPath, ref, ref1, ref2;
List.__super__.constructor.call(this);
this.userPackagesDirectory = path.join(config.getAtomDirectory(), 'packages');
this.devPackagesDirectory = path.join(config.getAtomDirectory(), 'dev', 'packages');

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace config.getAtomDirectory(),·'dev',·'packages' with ⏎········config.getAtomDirectory(),⏎········"dev",⏎········"packages"⏎······

Suggested change
this.devPackagesDirectory = path.join(config.getAtomDirectory(), 'dev', 'packages');
this.devPackagesDirectory = path.join(

(function() {
var apm, child_process, fs, path, temp;

child_process = require('child_process');

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace 'child_process' with "child_process"

Suggested change
child_process = require('child_process');
child_process = require("child_process");


_ = require('underscore-plus');

plist = require('@atom/plist');

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace '@atom/plist' with "@atom/plist"

Suggested change
plist = require('@atom/plist');
plist = require("@atom/plist");

this.syntaxVariables = SyntaxVariablesTemplate;
for (key in settings) {
value = settings[key];
replaceRegex = new RegExp("\\{\\{" + key + "\\}\\}", 'g');

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace 'g' with "g"

Suggested change
replaceRegex = new RegExp("\\{\\{" + key + "\\}\\}", 'g');
replaceRegex = new RegExp("\\{\\{" + key + "\\}\\}", "g");

return runs(function() {
var config;
expect(console.log).toHaveBeenCalled();
expect(console.log.argsForCall[0][0]).toMatch(/Not Disabled:\s*not-installed/);

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace /Not·Disabled:\s*not-installed/ with ⏎··········/Not·Disabled:\s*not-installed/⏎········

Suggested change
expect(console.log.argsForCall[0][0]).toMatch(/Not Disabled:\s*not-installed/);
expect(console.log.argsForCall[0][0]).toMatch(

@@ -0,0 +1,311 @@
// Generated by CoffeeScript 1.12.7
(function() {

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Insert ·

Suggested change
(function() {
(function () {


zlib = require('zlib');

_ = require('underscore-plus');

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace 'underscore-plus' with "underscore-plus"

Suggested change
_ = require('underscore-plus');
_ = require("underscore-plus");

@@ -0,0 +1,127 @@
// Generated by CoffeeScript 1.12.7
(function() {

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Insert ·

Suggested change
(function() {
(function () {

@DeeDeeG
Copy link
Member

DeeDeeG commented Nov 1, 2022

Just a brief comment, the node-gyp dependency being separate was only to resolve a quirk during testing -- depending on how how deeply or shallowly nested npm decides to hoist the node-gyp module under node_modules folder, its path can change. Whereas there is one spec that tries to copy node-gyp from its herd-coded path under node_modules.

Amin decided to pin a separate copy of node-gyp so it would always be hoisted to top-level. This was done over at the atom-community/apm repo.

Thes copy won't actually be used in real-world usage, only the copy required by npm will actually be used, so I recommend keeping it the same version so it will dedupe with npm's dependency of node-gyp.

If we really want newer node-gyp, in a more meaningful way, I have a branch over at atom-community/apm that bumps to npm 7, and here is a PR someone submitted to that repo to get ready for npm 8: atom-community/apm#123

@confused-Techie
Copy link
Member Author

This PR is far to out of date to even be considered from merging.

But the efforts made within are still perfectly valid, and encouraged to be retake to anyone who has time. Eventually I myself would really like to take another shot at this, but doing so properly:

  • Do not decaf spec and source at the same time
  • Breaking up each item into singular PRs to properly and easily review
  • Have codacy configured properly to not ruin any chance at successful PR comments (This one is already done)

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.

2 participants