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 the usage of sed commands #50

Merged
merged 24 commits into from
Mar 27, 2019
Merged

Remove the usage of sed commands #50

merged 24 commits into from
Mar 27, 2019

Conversation

DunnCoding
Copy link
Contributor

The behaviour of the sed commands differs slightly on macOS and Linux. As such the CLI currently isn't working on Linux.

To resolve this I've removed the usage of sed commands and instead I use the replace-in-file npm package. I also added a test to ensure we attempt to build the project kitura init creates to verify it can build successfully.

Copy link
Contributor

@ianpartridge ianpartridge left a comment

Choose a reason for hiding this comment

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

In general, this is good. I think it needs a whole heap of comments though about exactly what we're doing, why it's necessary etc.

let projNameLowercase = projName.toLowerCase();
    // Only contains alphanumeric characters.
    let projNameClean = projName.replace(/^[^a-zA-Z]*/, '')
        .replace(/[^a-zA-Z0-9]/g, '');
    // Does not contain uppercase letters.
    let projNameCleanLowercase = projNameClean.toLowerCase()
    let oldProjName = "Generator-Swiftserver-Projects";
    let oldProjNameLowercase = "generator-swiftserver-projects";
    let oldProjNameClean = "GeneratorSwiftserverProjects";
    let oldProjNameCleanLowercase = "generatorswiftserverprojects"

is just a big pile of WTF (not your fault I know!)

test.sh Outdated Show resolved Hide resolved
kitura-init.js Show resolved Hide resolved
kitura-init.js Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
@ianpartridge
Copy link
Contributor

@ddunn2 CI failed because Swift is not installed. Looks like the previous CI run failed too, but it wasn't reported as a failure? Can you figure out why that happened as well?

@DunnCoding
Copy link
Contributor Author

@ianpartridge Seems this repo was never set up to actually run Swift commands, so on Linux builds we don't have access to Swift for swift build

I'll look into the best way of adding this, I tried Package-Builder but it doesn't seem to want to play ball.

test.sh Outdated
@@ -30,7 +30,21 @@ echo "Installation complete"
rm "$PKG"

install_swift() {
eval "$(curl -sL https://swiftenv.fuller.li/install.sh)"
swiftFile = ".swift-version"
if [ -f "$file" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't right, you should be looking for ~/.swiftenv to determine if swiftenv is installed.

kitura-init.js Outdated
console.error(rename.stderr.toString());
process.exit(rename.status);
}
//Construct a regex expression to replace multiple occurances of oldProjName
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: occurrences

@@ -99,15 +100,23 @@ function cloneProject(url, branch) {
}

function renameProject() {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about a comment above this function to the effect of:

Projects created with kitura init start by cloning a template project, and renaming elements of that project to match the user's chosen project name (determined by the name of their current directory). We perform this renaming for three variants of the project's name: The original name as given, the same name with special characters stripped (as required by SwiftPM), and the stripped name also lowercased (used for example in the name of Docker containers generated by 'kitura build' and 'kitura run').

@djones6 djones6 merged commit 88fcf29 into master Mar 27, 2019
@djones6 djones6 deleted the fixIssue#42 branch March 27, 2019 09:38
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

3 participants