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

Proposed fix for the analyze step #102

Merged
merged 1 commit into from
Sep 25, 2021
Merged

Conversation

nfuller
Copy link
Contributor

@nfuller nfuller commented Jun 14, 2021

This commit avoids the hardcoded use of the ./dist directory in the diff
anaysis step, instead using the directory specified via a new
--working_dir flag. Without this, the ./dist directory is always used
(if it exists, otherwise failure).

The "dist/dist/" repeat has also been removed because it should no
longer be required.

This commit could have used the distDir variable instead, but a new
"working_dir" flag has been introduced instead. A generic name has been
chosen to enable its use for things besides diff analysis later if
needed. It's possible that several files in "dist" could be moved there
later if they're not needed in the distribution, but this has not been
attempted here.

I feel a new directory is reasonable because the files associated with
the previous release don't naturally fit with "downloads" (i.e. files
from OSM) or "dist" (the new distribution). In Android in the future, we
may commit dist + download for change history, but committing the
previous release file seems redundant as it also lives in dist/ under a
previous commit.

The flags documentation has been adjusted to try to make the
distinctions between directories clearer.

This commit avoids the hardcoded use of the ./dist directory in the diff
anaysis step, instead using the directory specified via a new
--working_dir flag. Without this, the ./dist directory is always used
(if it exists, otherwise failure).

The "dist/dist/" repeat has also been removed because it should no
longer be required.

This commit could have used the `distDir` variable instead, but a new
"working_dir" flag has been introduced instead. A generic name has been
chosen to enable its use for things besides diff analysis later if
needed. It's possible that several files in "dist" could be moved there
later if they're not needed in the distribution, but this has not been
attempted here.

I feel a new directory is reasonable because the files associated with
the previous release don't naturally fit with "downloads" (i.e. files
from OSM) or "dist" (the new distribution). In Android in the future, we
may commit dist + download for change history, but committing the
previous release file seems redundant as it also lives in dist/ under a
previous commit.

The flags documentation has been adjusted to try to make the
distinctions between directories clearer.
@szjozsef
Copy link

szjozsef commented Jun 29, 2021

If the working_dir will be accepted, I my opinion that workingDir should be used also in the function: getTzDistFilename instead of distDir and the files additions.json and removals.json should be saved there

@evansiroky
Copy link
Owner

@nfuller, I think this is a good idea. Also, I think @szjozsef has a good suggestion about including various other files currently written to the dist directory in this new working directory. I'm going to go ahead and merge this and then implement @szjozsef's recommendation such that only those files that are to be included with each release shall be written to the dist directory.

@evansiroky evansiroky merged commit 38bfbb2 into evansiroky:master Sep 25, 2021
@nfuller
Copy link
Contributor Author

nfuller commented Sep 27, 2021

SGTM. Thanks!

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