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 dash-generate-components for julia on case-insensitive filesystems #1567

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Mar 9, 2021

The problematic scenario occurs when

  • the case-lowered component name equals the module name
  • the --prefix option is left empty ''

See example: https://github.com/etpinard/dash-textarea-autocomplete/tree/main/src

Fixes etpinard/dash-textarea-autocomplete#7


The commit below prepends comp_ in jl component file names. There are other possible solutions to this problem, but I thought this one was the least-invasive. Let me know!

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

so that the component file name does not conflict with
the jl module filename in case-insensitive filesystems
when --prefix=''.
@alexcjohnson
Copy link
Collaborator

Nice catch @etpinard 🍻 I wonder though if we can scope this to just the problematic case, ie a component whose name (prefixed or not) matches the module name?

Although to play devil's advocate, in principle you could also imagine someone making a module MyThing with components named MyThing and comp_MyThing or naming the package comp_MyThing with components MyThing and comp_MyThing 😈 Perhaps the most bulletproof solution would be not a filename prefix but a subdirectory to put all the generated components into, like jl/mything.jl? That would also be slightly nicer for devs working on the JS side, to not have as many .jl files to look past. Just have to be sure this would still work in case the user makes their own src/jl/ dir and puts JS files into it.

cc @waralex

@etpinard
Copy link
Contributor Author

Perhaps the most bulletproof solution would be not a filename prefix but a subdirectory to put all the generated components into, like jl/mything.jl?

Yep, that's a nice idea. Looking forward to hearing what @waralex has to say here. I'll make the changes accordingly once you have reach a consensus. Cheers!

@waralex
Copy link
Contributor

waralex commented Mar 30, 2021

Perhaps the most bulletproof solution would be not a filename prefix but a subdirectory to put all the generated components into, like jl/mything.jl?

Yep, that's a nice idea. Looking forward to hearing what @waralex has to say here. I'll make the changes accordingly once you have reach a consensus. Cheers!

Yes, placing files with components in a subdirectory looks like the most reasonable solution.

- so that their paths never conflict with the julia module file
  in case-insensitive filesystems
- this also makes the src directory less noisy (especially for
  devs that do not care about julia)
@etpinard
Copy link
Contributor Author

etpinard commented Apr 1, 2021

Perhaps the most bulletproof solution would be not a filename prefix but a subdirectory to put all the generated components into, like jl/mything.jl?

Attempted in 90dfd9f

I believe this commit makes this block

# the Julia source directory for the package won't exist on first call
# create the Julia directory if it is missing
if not os.path.exists("src"):
os.makedirs("src")

obsolete. Alternatively, we could move the os.makedirs("src/jl") statement into generate_module, but then we would need to patch generate_components to make src/jl/ available for generate_struct_file which is currently called before generate_module.

Just have to be sure this would still work in case the user makes their own src/jl/ dir and puts JS files into it.

I tested two cases:

with:

. venv/bin/activate
pip install -r requirements.txt 
pip install ../../dash
npm run build
julia --project -e 'import Pkg; Pkg.add("DashHtmlComponents")'
julia --project usage.jl

where the build script got generated from dash-component-boilerplate.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Agreed that we should be able to 🔪 os.makedirs("src") - then this looks good! 💃

@alexcjohnson alexcjohnson merged commit efc42b4 into plotly:dev Jun 8, 2021
@etpinard etpinard deleted the etpinard/jl-compgen_filename-case-insensitive-fs branch June 8, 2021 15:09
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.

Precompilation issue on Windows
4 participants