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

Support getfilePath api and handle same function identifier #51

Closed
boy1583 opened this issue Jul 21, 2021 · 3 comments
Closed

Support getfilePath api and handle same function identifier #51

boy1583 opened this issue Jul 21, 2021 · 3 comments

Comments

@boy1583
Copy link

boy1583 commented Jul 21, 2021

We used bin2cpp to process our SPA single page application resources in a folder (html\css\js).

We have two issues.

  1. For the generation method in a folder, we can‘t get the path of the original file. So we created a new API called getFilePath in File class.
    E.g:
virtual const char * getFilename() const { return "app.4581e082.css"; }
virtual const char * getFilepath() const { return "static/css/app.4581e082.css"; }
  1. call GetFilenameWithoutExtension double times in processInputDirectory, which causes the following files to be overwritten (both a.h):
static/js/A.123.js
static/css/A.456.css

We remove one GetFilenameWithoutExtension call in getFunctionIdentifierFromPath.

  1. The following situation may lead to ths same function identifier (e.g. class A123), leading to compilation errors (redeclaration)
static/js/A.123.js
static/css/A.123.css

We use a map to save identifier times, which is solved by appending increase index, such as class A123_1 and class A123_2, although it is not very elegant.

@end2endzone
Copy link
Owner

Hi. Thank you for showing interest in bin2cpp. Apologize for the late response.

Regarding your first issue, I think it is a really good idea. I really like it.
The getFilePath() api call makes the function getFilename() not required anymore. However, it should be kept for backward compatibility reason.
I think we could also implement such functionality with a new filepath command line argument.
When specified, this path could be used to implement the getFilePath() api and the existing getFilename() api.
For example:

bin2cpp --noheader --override --file=static\css\A.123.css --headerfile=A_123.h --filepath="static\css\A.123.css" --output=.\outdir --identifier=A123 --managerfile=FileMgr.h --registerfile

When using --dir argument, the command could forward the actual related path "static\css\A.123.css" to the filepath argument.


I have looked more deeply about your second issue.
You are right, it seems to be a bug located in main.cpp, line 575.

We remove one GetFilenameWithoutExtension call in getFunctionIdentifierFromPath

I disagree with your proposed solution. In my opinion, based on its name, the function getFunctionIdentifierFromPath() is obviously expecting a file path instead of only a filename. I think I made an error in commit be71f9f when fixing #42 in which I renamed the function getIdentifier() to getFunctionIdentifierFromPath(). I think the solution would be to remove the call to GetFilenameWithoutExtension() on line 575 before calling getFunctionIdentifierFromPath(). In my preliminary tests, it seems to solve the issue.


The problem you mention at your third issue is easily resolved with the the fix explained above.
The correction gives the identifiers getA123File() and getA456File() generated from files A.123.js and A.456.css.


I am really happy that you found out about these problem.
I am also open to contribution. Would you like to implement the fixes?

  1. Implement a --filepath command line that is used to specify the returned value when calling getFilePath() function.
  2. Remove the call to GetFilenameWithoutExtension() on main.cpp, line 575.

I have looked at the changes in your forked repository. More specifically, at commit 02fbf0908e2fb9bc381608072a942bb9b53bdb08.
I think there are more changes than what is actually required.
If you would like to make the changes yourself and contribute to the project, please tell me so that I do not work on them.
If not, I am also fine and I will implement the feature in the next month.

end2endzone added a commit that referenced this issue Sep 29, 2021
…ure of the input directory in the output directory. This feature is required in preparation for fixing #51 and #56.
end2endzone added a commit that referenced this issue Oct 1, 2021
* feature_issue56:
  Added double quotes in generate_test_files.sh.in to prevent 'Permission Denied' error.
  Fixed invalid script commands in `generate_test_files.sh.in`. Modified cmakelists.txt to create target `build_test_files` which makes sure that all required unit tests files are created before building bin2cpp_unittest target.
  * Fixed issue #56: Problem with certain filenames
  Implemented getUniqueFilePath() and unit tests.
  Implemented `pathSplit()` and `pathJoin()` with unit tests.
  Implemented --keepdirs which allows to keep the same directory structure of the input directory in the output directory. This feature is required in preparation for fixing #51 and #56.
  Updated product to version 3.0.0 since the api is now broken. Moved `filter()` and `getFunctionIdentifierFromPath()` from `main.cpp` to `common.h` so that it can be reused and property tested. Updated `getFunctionIdentifierFromPath()` and `getUniqueFunctionIdentifierFromPath()` to properly generated identifiers from a file path. Created unit tests as well.
@end2endzone end2endzone added this to the 0.7.0 milestone Oct 1, 2021
@end2endzone
Copy link
Owner

I did not received feedback from you. I have looked at you commit in your fork repository. Like I said: "I think there are more changes than what is actually required".

I have now fixed new issues since your last commit and I do not think your original changes still applies. However, since this is your idea, I would like to give you the opportunity to contribute to this repository. Here is what I would propose for the new changes:

  1. Update the public api with a new getFilePath() function. This change is exactly like you implemented here.
  2. Implement a --reportedfilepath command line that is used to specify the returned value when calling getFilePath() function. Also modify ARGUMENTS structure.
  3. Create new methods in BaseGenerator for storing the value specified with --filepath. I think the names you used (setFilePathInDir() and getFilePathInDir()) are not ideal. The problem is the Dir postfix. My point is that the option --reportedfilepath is also available when the user does not specify the --dir command line. In other words, if a user calls bin2cpp with --file and --reportedfilepath, using the postfix Dir is out of context. I think the names setReportedFilePath() and getReportedFilePath() are more appropriate. The names also matches the format setXXXFilePath() like setInputFilePath().
  4. Modify the code so that when using --keepdirs, the code automatically implements --reportedfilepath.
  5. Create unit tests to validate the new behavior.

I would be available to start working on these changes tomorrow. Please advise if you agree to make the changes. I you would prefer to not contribute, I am also fine with that.
Thank you.

end2endzone added a commit that referenced this issue Oct 9, 2021
* feature-issue51:
  Updated documentation for --reportedfilepath option. #51
  * Fixed issue #51: Support getfilePath api and handle same function identifier.
  Fixed invalid characters in generate_test_files.sh.in.
  Reordered unit test generated directories list in bin2cpp\test\bin2cpp_unittest\CMakeLists.txt.
  Implemented unit tests for issue #51.
  Deprecated method `getFilename()` in favor to `getFileName()`. This is to be more inline with getFilePath().
  Fixed compilation of generated code for method `getFileName()`
  Removed directories from generated files in project bin2cpp_unittest.
  - Implemented support for `--reportedfilepath` command line. - Implemented automatic reported path when using `--dir` mode. - Modified IGenerator interface to add `setReportedFilePath()` and `getReportedFilePath()`. - Modified all generators to update the implementation of `getFilename()` and `getFilePath()` of the public api.
@end2endzone
Copy link
Owner

Feature and documentation is completed.

end2endzone added a commit that referenced this issue Oct 19, 2021
Header file default value: Input file name (without extension)
Identifier default value: Based on input file. Format is "NameExt"
For #51 and #58.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants