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

Develop #634

Closed
wants to merge 30 commits into from
Closed

Develop #634

wants to merge 30 commits into from

Conversation

xyang-github
Copy link

Originally, the program would throw an error if a non-latin name was entered as the foreign name. This was due to a regex that resulted in a blank class name.

My fix circumvents this issue, by explicitly asking the user for a class name instead of deriving it completly from the formal name. Added a make_class_name method and a validate_class_method. Originally had planned to make the candidate class_name to be from the app_name, but later decided to keep it with the formal_name. If a non-latin formal name is entered, then the program will prompt the user to enter an appropriate class name via validation. The regex used should be consistent with camel case format. Two test files, one for each new class_name method, was created and passed when ran.

During this process, also realized that the regex for the app_name isn't sufficient. I changed the regex statement so that the app name cannot start with a digit or a capitalized letter. If the formal name is non-latin, then the make_app_name method will return a blank string, which will raise a validation error if user doesn't enter anything. Updated the test pages, and the changes made passed.

Reference to issue #612

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

…formal name when making the default app name.
…umber. Also updated the value error to reflect this.

git status
…, due to the restrictions in the naming scheme. Removed make_class_name as additional formatting is not needed from the app name to make a valid class name. Also moved the instantiation of class_name to appear below validation of the app_name.
…ional if a blank string is created from the regex, which may happen if the formal name was entered with only non-latin characters. Will return a statement that an example cannot be given, which will require the user to type in a candidate name into the command line.
… validates input in the validate_class_name method.
…strictions of app_name. Replaced in new.py with regex to only allow app_names that start with a lowercase letter.
- added test to invalid testing: name with a leading digit, blank, non-latin character, leading capital letter. Changes passed all tests.
…generated, just like with the app_name. This should help with the testing that requires some default value.
…f statement to evaluate if the first character is a digit will throw an error if the formal name is non-latin.
… the validate_app_name, with some differences.
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This is on the right track; I've highlighted a couple of areas that need attention, but this is starting to look pretty good!

src/briefcase/commands/new.py Outdated Show resolved Hide resolved
src/briefcase/commands/new.py Outdated Show resolved Hide resolved
tests/commands/new/test_make_class_name.py Show resolved Hide resolved
src/briefcase/commands/new.py Outdated Show resolved Hide resolved
…tement. This is to create camel case of words that were otherwise in lower case. Updated test scenario, which this passes.
…. Class names should be in camel case, so created a test where it converts from lower case words to camel case.
…of latin and non-latin characters. Passes test.
@xyang-github
Copy link
Author

@freakboy3742 Apologies it has taken so long. I am reviewing the reasons why the checks were not successful.

This pull request was closed.
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