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(utils): unify common utility scripts #18059

Merged
merged 3 commits into from
Sep 26, 2019

Conversation

ojeytonwilliams
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams commented Aug 28, 2018

Unify common scripts like dasherize.

@raisedadead raisedadead changed the base branch from staging to master September 26, 2018 12:07
@raisedadead raisedadead added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Sep 26, 2018
@raisedadead raisedadead added status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Sep 27, 2018
@paulywill
Copy link
Member

@raisedadead @Bouncey Why is the PR still open if labeled blocked?

@paulywill paulywill closed this Oct 14, 2018
@paulywill paulywill reopened this Oct 14, 2018
@paulywill
Copy link
Member

Opps... wrong button

@raisedadead
Copy link
Member

raisedadead commented Oct 14, 2018

Hi @ojeytonwilliams @paulywill this is change that we are evaluating for inclusion.

We have merged the learn platform in this main repo. So, it needs to be decided where we need to put up some common utils like such as this file.

We haven't gotten around to this, hence the blocked status. The action is on the @freeCodeCamp/dev-team for now.

Thanks.

@thecodingaviator
Copy link
Contributor

@raisedadead What are we thinking about this Pr a.t.m?

@RandellDawson
Copy link
Member

@ValeraS Is this PR still needed, since you made the recent changes to dasherize? The only other thing I see is changes to nameify function.

@ojeytonwilliams
Copy link
Contributor Author

@RandellDawson I think an updated version of this PR is needed, since there are still several instances of dasherize and nameify. dasherize is a bit tricky because some of which are slightly different!

Nonetheless, if they can be unified, presumably they should be. Perhaps in a new common utils directory.

@RandellDawson RandellDawson added status: merge conflict To be applied to PR's that have a merge conflict and need updating and removed status: merge conflict To be applied to PR's that have a merge conflict and need updating labels Jun 4, 2019
@raisedadead
Copy link
Member

Are we able to revisit or close this @ojeytonwilliams ?

@ojeytonwilliams
Copy link
Contributor Author

Well, this is stale to the point of useless, but I still think we should DRY out this stuff.

I've taken another look at the three dasherize functions and they all have exactly the same functionality. They're just written slightly differently.

Is there any reason not to just put a single util in the root dir and have the client, server and curriculum all use that?

@raisedadead
Copy link
Member

Is there any reason not to just put a single util in the root dir and have the client, server and curriculum all use that?

There is none. Please go ahead with the refactor if you are interested.

@raisedadead raisedadead removed the status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. label Sep 25, 2019
@ojeytonwilliams ojeytonwilliams requested a review from a team September 25, 2019 18:10
@gitpod-io
Copy link

gitpod-io bot commented Sep 25, 2019

@raisedadead raisedadead changed the title fix(sync-utils): Change utils to match learn fix(utils): unify common utility scripts Sep 25, 2019
@raisedadead raisedadead removed the status: merge conflict To be applied to PR's that have a merge conflict and need updating label Sep 25, 2019
@raisedadead raisedadead added the scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. label Sep 25, 2019
@raisedadead
Copy link
Member

raisedadead commented Sep 26, 2019

Hi @ojeytonwilliams I like this, and going forth we should probably using this structure for our utils. Do you mind throwing some unit tests in?

Once you do please, feel free to hit the merge button.

@ojeytonwilliams
Copy link
Contributor Author

@raisedadead are these tests what you had in mind? They made me realise that " a title " becomes "-a-title--", which doesn't look great. Perhaps we should trim the strings first.

@raisedadead
Copy link
Member

These tests are perfect and as you seem to have found a use case where we do need to trim the spaces.

Copy link
Member

@RandellDawson RandellDawson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@raisedadead raisedadead left a comment

Choose a reason for hiding this comment

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

You sir rock!

@raisedadead raisedadead merged commit f159645 into freeCodeCamp:master Sep 26, 2019
@ojeytonwilliams ojeytonwilliams deleted the fix/dasherize branch September 26, 2019 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants