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

update readme.md with buildConfig detail #359

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hiepxanh
Copy link

Platforms affected

android, ios, browser

What does this PR do?

just update readme to clarify that have ~ can realate to HOME path enviroment variable

What testing has been done on this change?

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@janpio
Copy link
Member

janpio commented Nov 16, 2018

Why is this so relevant here that it should explicitly be mentioned? There are loads of other possible ENV variables that can point to all kinds of interesting folders. We don't mention those here either.

Related: apache/cordova-docs#909

@hiepxanh
Copy link
Author

hiepxanh commented Nov 18, 2018

I already answer on this topic sir, [fix] better support for window, sorry for copy text have cap . Let discussion about this.

@brodycj
Copy link
Contributor

brodycj commented Nov 18, 2018

Thanks @hiepxanh for pointing it out and proposing the idea. I have some doubts, though.

This seems to be specific to the Android platform, and from apache/cordova-android#563 (comment), and I would agree with apache/cordova-android#563 (comment) that this may not what we want to support in the future.

Another thing is that I think ~ is not universally supported on Windows cmd shell (not sure though).

I think we should look for a way to support specifying a path based on the home directory in a way that works consistently across all Cordova platforms and all development systems, and get it documented if we fix it.

I recommend that you raise a single issue in https://github.com/apache/cordova/issues to explain the situation and propose one or more alternative solutions for discussion.

P.S. I would also appreciate it if people would avoid using words with "all caps", even in a link, since it is generally interpreted as a form of shouting. I would recommend that you consider wrapping the text with ** on both sides to emphasize it in bold, works for links as well. Some recommended reading is here.

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

See discussion at apache/cordova-android#563

@janpio
Copy link
Member

janpio commented Nov 18, 2018

Thanks @brodybits for ignoring the explicit wish of @hiepxanh to discuss this in the PR and with that splitting the discussion to several places. A lot more impolite than using all caps in a harmless link that from context alone clearly is not shouting.

@brodycj
Copy link
Contributor

brodycj commented Nov 18, 2018

@hiepxanh @janpio @raphinesse I am fine to discuss approach and solution here, apache/cordova-android#563, #359, or wherever else you guys want to discuss it. I would favor using untildify as suggested in apache/cordova-android#563 (comment), and I would like to see a similar solution applied to all platforms as discussed in apache/cordova-android#563. My comments are my opinions, please feel free to agree or disagree as you like.

@hiepxanh
Copy link
Author

hiepxanh commented Nov 18, 2018

I'm so sorry @janpio @brodybits, I don't mean to shout or impolite, sorry for my unintended action. I just copy that title text and it have with cap. I was fix it. and I agree with sir @brodybits that solution to use untildify. it will be very helpful. My suggest is just an template solution when I see that no way to this. I'm very happy to that member concern about this and discuss with me. Should I close this issue if we will have an other solution soon?

@janpio
Copy link
Member

janpio commented Nov 18, 2018

I created apache/cordova#48 as a proper discussion issue about this. I hope I got everything.

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

Pending conclusion of apache/cordova-android#563 and apache/cordova#48

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