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

ARIA Download NISAR GUNWs #394

Closed
wants to merge 9 commits into from
Closed

Conversation

bbuzz31
Copy link
Collaborator

@bbuzz31 bbuzz31 commented Apr 9, 2024

add download support for NISAR GUNWs

@bbuzz31 bbuzz31 marked this pull request as ready for review April 9, 2024 22:12
Copy link
Collaborator

@alexfore alexfore left a comment

Choose a reason for hiding this comment

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

I added some questions and a few style points

tools/bin/ariaDownload.py Outdated Show resolved Hide resolved
tools/bin/ariaDownload.py Outdated Show resolved Hide resolved
en0 = self.args.end + relativedelta(months=-3)

if self.args.mission.upper() == 'S1':
dct_kw = dict(dataset='ARIA S1 GUNW',
Copy link
Collaborator

Choose a reason for hiding this comment

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

misalignment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure what it's supposed to be

Copy link
Collaborator

Choose a reason for hiding this comment

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

The llines are not aligned correctly

Copy link
Collaborator

Choose a reason for hiding this comment

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

    dct_kw = dict(dataset='ARIA S1 GUNW',
                    processingLevel=asf_search.constants.GUNW_STD,

dataset and processingLevel should be aligned on subsequent lines

tools/bin/ariaDownload.py Outdated Show resolved Hide resolved
tools/bin/ariaDownload.py Outdated Show resolved Hide resolved
@alexfore
Copy link
Collaborator

alexfore commented Apr 11, 2024

I've asked repeatedly for you to pay attention to the style of the code which you are committing. You have not done that so far. Please look at what you have done and ask yourself:

  • Is what I have committed PEP8 compliant?
  • Is the use of whitespace consistent with the rest of the code in this module?
  • Is what I have committed improving the software or adding more issues that need to be fixed in the future? (technical debt)

I think that style is extremely important. From my point of view imposing style requirements on this code base is the first step to making it a maintainable code base. If the main developers on this project do not agree I will not be able to help

It will be worth your time to memorize everything here: https://peps.python.org/pep-0008/

@bbuzz31
Copy link
Collaborator Author

bbuzz31 commented Apr 11, 2024

I implemented all your requested changes. If you have more requests, point them out.

@jhkennedy
Copy link
Collaborator

I've asked repeatedly for you to pay attention to the style of the code which you are committing. You have not done that so far. Please look at what you have done and ask yourself:

* Is what I have committed PEP8 compliant?

* Is the use of whitespace consistent with the rest of the code in this module?

* Is what I have committed improving the software or adding more issues that need to be fixed in the future? (technical debt)

I think that style is extremely important. From my point of view imposing style requirements on this code base is the first step to making it a maintainable code base. If the main developers on this project do not agree I will not be able to help

It will be worth your time to memorize everything here: https://peps.python.org/pep-0008/

@alexfore, there seems to be a lot of unnecessary back and forth related to this that should be handled by CI/CD linting checks -- it's much better for everyone involved for a bot to be the arbiter.

I suggest spending the effort on setting up a github action for flake8/pylint/ruff.

@alexfore
Copy link
Collaborator

I would also love to integrate automated pep8 checks -- because I never want to have this conversation again. I do not like telling people that their code style is messy or inadequate, this clearly leads to conflict.

However, the fact remains that this PR has still contains pep8 violations -- whitespace and alignment. My comments above still stand and I want to see them addressed before I will approve this PR. If someone is unwilling to improve I do not know what to say to that. Maybe use pylint, pycodestyle, or flake8 and try to learn what the issues are. Me telling you or doing it myself is not going to help other people learn.

On my end I will figure out how to integrate the style checks I want in another PR.

@dbekaert
Copy link
Collaborator

dbekaert commented Apr 12, 2024

Hi @alexfore @jhkennedy I like the idea of a bot doing these sort of checks. I think they might even come with suggested fixes etc. Lets add that support in an upcoming PR and perhaps we can let the code-base run through it?

I would like to get the stable test suite in place, the dependency issue fixed for stability. This will allow to check against changes and see if something breaks. With that in place we could route the code through the both for suggested changes?

@alexfore
Copy link
Collaborator

alexfore commented Apr 12, 2024

I am going to close this PR. We can open a new PR for this branch once I've integrated pep8 auto checks into this repo, and the author has addressed the issues raised by those checks.

@alexfore alexfore closed this Apr 12, 2024
@bbuzz31 bbuzz31 reopened this May 10, 2024
@pep8speaks
Copy link

Hello @bbuzz31! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 36:80: E501 line too long (80 > 79 characters)
Line 37:22: E131 continuation line unaligned for hanging indent
Line 41:80: E501 line too long (89 > 79 characters)
Line 318:9: E266 too many leading '#' for block comment
Line 318:9: E303 too many blank lines (2)
Line 320:12: E221 multiple spaces before operator
Line 324:29: E127 continuation line over-indented for visual indent
Line 325:29: E127 continuation line over-indented for visual indent
Line 326:29: E127 continuation line over-indented for visual indent
Line 327:29: E127 continuation line over-indented for visual indent
Line 328:29: E127 continuation line over-indented for visual indent
Line 329:29: E127 continuation line over-indented for visual indent
Line 330:13: E124 closing bracket does not match visual indentation
Line 331:29: E116 unexpected indentation (comment)
Line 338:13: E266 too many leading '#' for block comment
Line 341:29: E126 continuation line over-indented for hanging indent
Line 349:25: E126 continuation line over-indented for hanging indent
Line 351:27: E225 missing whitespace around operator

@bbuzz31 bbuzz31 marked this pull request as draft May 10, 2024 23:36
@alexfore
Copy link
Collaborator

This PR needs to be abandoned. These code changes need to start from the dev branch after I've merged dev_refactor.

@alexfore alexfore closed this May 11, 2024
@alexfore alexfore deleted the dev_refactor-ariaDL branch May 14, 2024 16:00
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.

None yet

5 participants