Skip to content

feat(build): allow create-class to drop classes into cwd #3193

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

Merged
merged 1 commit into from
Jun 25, 2022

Conversation

ThorstenSuckow
Copy link
Contributor

create-class creates class files in current working directory

This PR introduces several new enhancements to the create-class script:

  1. new option for dropping classes directly into the cwd: -d
  2. option for specifying the app-source folder for computing namespace when -d is used: -s
  3. script available as binary via npx neo-cc (test w/ npm i -g for repository clones)
  4. several minor enhancements for original source related to path computing

Changes

The original implementation remains unchanged regarding functionality, some lines had to be shifted and re-arranged into conditional statements given the new option the script provides. It was also necessary to distinguish between - and further reuse - the original cwd- and __dirname-variables, so the script can now be run outside of the neo-folder.

Features

-d Option

The -d options stands for drop and advises the script to run in a none-interactive environment. It will compute the namespace based on the current working directory up to the directory that is either used from the value found in the script's sourceRootDirs-array, or specified with the -s-option.

Example:

The user has an application in /neo/apps/acme and wants to create a new class for acme.view.masterDetail.TableView.

cd /neo/apps/acme/view/masterDetail
npx neo-cc -c TableView -b component.Base -d

results in a file TableView.mjs created at /neo/apps/acme/view/masterDetail with the following contents:

import Base from '../../../../src/component/Base.mjs';

/**
 * @class acme.view.masterDetail.TableView
 * @extends Neo.component.Base
 */
class TableView extends Base {
    static getConfig() {return {
        /*
         * @member {String} className='acme.view.masterDetail.TableView'
         * @protected
         */
        className: 'acme.view.masterDetail.TableView',
        /*
         * @member {Object} _vdom
         */
        _vdom:
        {}
    }}
}

Neo.applyClassConfig(TableView);

export default TableView;

Note, that apps is by default mentioned in the sourcesRootDir of the script. The user can overwrite the directory where the sources (and therefore top-level namespaces reside) by using the -s option:

Example:

The user has an application in /neo/FOLDER/acme and wants to create a new class for acme.view.masterDetail.TableView.

cd /neo/FOLDER/acme/view/masterDetail
npx neo-cc -c TableView -b component.Base -d -s FOLDER

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the dev branch, not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@ThorstenSuckow ThorstenSuckow changed the title feat(build): allow create-class to drop classes in cwd feat(build): allow create-class to drop classes into cwd Jun 24, 2022
@tobiu
Copy link
Collaborator

tobiu commented Jun 25, 2022

Hi @ThorstenSuckow, of course i will accept the PR :)

Just 2 comments:

  1. I don't think we should add a license on a "per file" basis. We have the LICENSE file on the top level of the repo, which github and npm recognises. Adding it inside every file would increase the repo-size quite a bit and if we want to change the wording a little bit at some point, it would be quite some effort. We could create a new ticket to discuss about it though.
  2. App namespaces are most likely written in PascalCase. E.g. acme would probably be AcMe or something similar. Since the folder name of an app does not need to know about uppercase letters, there are 2 files: https://github.com/neomjs/neo/blob/dev/buildScripts/webpack/json/myApps.template.json and a version without the .template as soon as you create a new app. this file also exist inside the workspace scope. As a follow-up ticket, we should parse the file(s) and adjust the class names accordingly.

@tobiu tobiu merged commit eff57fe into neomjs:dev Jun 25, 2022
@ThorstenSuckow
Copy link
Contributor Author

  1. I don't think we should add a license on a "per file" basis. We have the LICENSE file on the top level of the repo, which github and npm recognises. Adding it inside every file would increase the repo-size quite a bit and if we want to change the wording a little bit at some point, it would be quite some effort. We could create a new ticket to discuss about it though.

ACK.

  1. App namespaces are most likely written in PascalCase. E.g. acme would probably be AcMe or something similar. Since the folder name of an app does not need to know about uppercase letters, there are 2 files: https://github.com/neomjs/neo/blob/dev/buildScripts/webpack/json/myApps.template.json and a version without the .template as soon as you create a new app. this file also exist inside the workspace scope. As a follow-up ticket, we should parse the file(s) and adjust the class names accordingly.

I don't think I understand you correctly - how does this affect the proposed changes?

Also, the usage of PascalCase instead of camelCase in namespace parts worries me since I'm not used to it ;) , but I will create a discussion out of it.

@Dinkh
Copy link
Contributor

Dinkh commented Jul 11, 2022

I am in for PascalCase ;)
The app folder is lowercase.
The NameSpace is PascalCase.

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.

3 participants