plotly / dash-daq

Control components for Dash
MIT License
141 stars 40 forks source link

Gauge/bug fix #148

Closed Karan-S-Mittal closed 3 years ago

Karan-S-Mittal commented 3 years ago

These are the issues, I worked on

Before: After: chrome_3aFCeJTkLv

Thank you so much for reviewing.

alexcjohnson commented 3 years ago

Thanks @Karan-S-Mittal - the code changes look promising, I'll review those shortly, but something odd happened with the build process: the quote characters in the build:r package script were treated as part of the input, so we ended up with a bunch of duplicate files like R/'daq'Gauge.R in addition to the expected R/daqGauge.R, and man/'daq'Gauge.Rd in addition to man/daqGauge.Rd. Also in the new .Rd files there are a whole lot of ^M characters (CR) - not at all line breaks, but at many of them, and occasionally not at a line break at all, like here.

I'm assuming these are all a result of running the same commands we've used in the past, but you're on Windows and most of the rest of us develop on Mac or Linux. In a sense this is good, an opportunity for us: it should work fine to build components on Windows, we just need to figure out why it didn't.

The good news is building the Python components seems to have worked perfectly. There are changes in these files, but that's to be expected because we've updated the Python component generator since dash-daq was most recently rebuilt.

So the two routes we could take are:

The latter is preferable if you're up for it. The place this is all happening is https://github.com/plotly/dash/blob/dev/dash/development/_r_components_generation.py. Re: CR, if we're loading the source files correctly Python should have already converted \r\n to \n for us, but perhaps we need to explicitly replace os.linesep with \n in some cases? And re: the single quotes, maybe we want to solve this up a level? In principle all the args here should have quotes removed if any made it that far.

Karan-S-Mittal commented 3 years ago

Thank you for sharing the build error, I have been searching on this. I would like to work on making the repository more contributor-friendly on windows and try to make it more cross-platform. Again, Thank you for sharing the files with me, I will work on implement the changes as you suggested and update the PR.

Karan-S-Mittal commented 3 years ago

Hey @alexcjohnson, #124 is resolved.

Before:

chrome_sc7qnQVD8Q

After:

issue 124

build error

this time I used Github's codespaces as a build environment(Linux) but for some reason, the build process is showing an error of unauthorized. attaching an image of what I can see.

chrome_8ewmxXgcfo

alexcjohnson commented 3 years ago

Get rid of the context in circleci config - like https://github.com/plotly/dash-bio/pull/539/files It's unnecessary and prevents PRs from forks from running CI tests.

Karan-S-Mittal commented 3 years ago

Thanks @Karan-S-Mittal - the code changes look promising, I'll review those shortly, but something odd happened with the build process: the quote characters in the build:r package script were treated as part of the input, so we ended up with a bunch of duplicate files like R/'daq'Gauge.R in addition to the expected R/daqGauge.R, and man/'daq'Gauge.Rd in addition to man/daqGauge.Rd. Also in the new .Rd files there are a whole lot of ^M characters (CR) - not at all line breaks, but at many of them, and occasionally not at a line break at all, like here.

I'm assuming these are all a result of running the same commands we've used in the past, but you're on Windows and most of the rest of us develop on Mac or Linux. In a sense this is good, an opportunity for us: it should work fine to build components on Windows, we just need to figure out why it didn't.

The good news is building the Python components seems to have worked perfectly. There are changes in these files, but that's to be expected because we've updated the Python component generator since dash-daq was most recently rebuilt.

So the two routes we could take are:

  • Figure out a way to run on Windows with the existing code, and document this
  • Fix the code so it works correctly on Windows whichever way you run it.

The latter is preferable if you're up for it. The place this is all happening is https://github.com/plotly/dash/blob/dev/dash/development/_r_components_generation.py. Re: CR, if we're loading the source files correctly Python should have already converted \r\n to \n for us, but perhaps we need to explicitly replace os.linesep with \n in some cases? And re: the single quotes, maybe we want to solve this up a level? In principle all the args here should have quotes removed if any made it that far.

Hey Alex, I have checked the dash renderer and found that if we can add the strip function in this https://github.com/Karan-S-Mittal/dash/blob/d5df9a704049169e29521e5c419973b9d16bd972/dash/development/_r_components_generation.py#L735-L738 we can resolve the files error. here are some snapshots of before and after I made the change of file structure.

Before modification

WindowsTerminal_Mbc2LwgOOa

After the prefix variable is stripped for single quotes

WindowsTerminal_tRExTt4s2I

Though there are some platform issues with Dash Repository as well on windows and therefore cannot update the commit. Can you please look at this simple addition and let me know if I am moving in the right direction.

image

The current build fails are a part of different errors that I wasn't able to track down. I am exploring and learning Circle CI more for that currently.

Karan-S-Mittal commented 3 years ago

Issues Resolved in this PR update #113, #144, and thermometer #64.

Issue #113

the issue was to show the true value of the sensor which was being limited to the value to max attribute. issue-113

Issue #144

the issue was similar to issue #113 but related to the tank.

Issue #64

Also removed the context as you mentioned in a previous comment.

alexcjohnson commented 3 years ago

@Karan-S-Mittal the CI build error is a result of trying to build Dash from source, which has changed with Dash 2. I guess we originally did this so that the dash-daq tests would run against the tip of dash's dev branch, but I think that has outlived its usefulness and we should just install the latest released version of dash. So that would mean changing:

https://github.com/plotly/dash-daq/blob/01b4bd8d708c696fbe8f2d1c20365a950fb91106/.circleci/config.yml#L32-L35

To just

pip install --upgrade dash[dev,testing]

(probably don't even need the --upgrade, just included in case there's some caching I'm not seeing)

alexcjohnson commented 3 years ago

Re: extra single-quotes: Good to know that patching format_fn_name fixes it. But dash-daq only uses the one extra arg --r-prefix in its build command, there are many others which presumably suffer the same problem here:

https://github.com/plotly/dash/blob/d21a3654443bcab467a3db38a43320c08f8c7d0b/dash/development/component_generator.py#L210-L218

So I think the best solution would be to strip quotes from all of them right at that point.

Then there's still the question of how the CR characters got inserted. The easy solution would be to just delete them all at the end, but it would be better if we could figure out where they came in and prevent them in the first place.

Karan-S-Mittal commented 3 years ago

Issues Resolved in this PR update #112, and #133

Issue #133 Resolved

issue 133

Issue #112 Resolved

issue 112

@alexcjohnson, can you review the PR now, and if possible can we merge and close the listed issues.

Karan-S-Mittal commented 3 years ago

Total Issues Resolved