Skip to content

Conversation

@jveitchmichaelis
Copy link
Collaborator

Adds two copilot instructions (for core + tests) following guidance here: https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions

Should hopefully improve generated outputs + reduce our load fixing things like tests.

@jveitchmichaelis jveitchmichaelis marked this pull request as ready for review October 23, 2025 21:34
@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.47%. Comparing base (e20f94f) to head (c8e7cc5).
⚠️ Report is 26 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1183      +/-   ##
==========================================
+ Coverage   87.43%   87.47%   +0.03%     
==========================================
  Files          20       20              
  Lines        2538     2586      +48     
==========================================
+ Hits         2219     2262      +43     
- Misses        319      324       +5     
Flag Coverage Δ
unittests 87.47% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@ethanwhite ethanwhite left a comment

Choose a reason for hiding this comment

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

Looks good. Is there an instruction that can be used to block the prevent the "Initial plan" empty commit?

Other than that just one typo fix and one minor suggestion.

I'll be curious to see how this improves things.

@ethanwhite
Copy link
Member

Is there an instruction that can be used to block the prevent the "Initial plan" empty commit?

Spent a few minutes trying to answer this myself and couldn't find anything.

@jveitchmichaelis
Copy link
Collaborator Author

Most I can find is "no" https://github.com/orgs/community/discussions/160091

@jveitchmichaelis
Copy link
Collaborator Author

One thing I'm not sure about is whether it would be better to use the AGENTS.md convention rather than explicitly target copilot, which can read both.

@ethanwhite
Copy link
Member

One thing I'm not sure about is whether it would be better to use the AGENTS.md convention rather than explicitly target copilot, which can read both.

I like the more general approach. Looks like it's a little more widely adopted. Will work locally in some cases and I always prefer minimizing GitHub specific stuff when possible for future flexibility.

@bw4sz
Copy link
Collaborator

bw4sz commented Oct 27, 2025

i'm happy, looks like @ethanwhite needs to clear.

@jveitchmichaelis
Copy link
Collaborator Author

jveitchmichaelis commented Oct 27, 2025

Standby - we might go for the Agents.md approach, so it also works with Cursor/Claude/etc. (only requires moving files, and will fix content suggestions above)

@bw4sz
Copy link
Collaborator

bw4sz commented Oct 30, 2025

I don't know if its useful, but here are my copilot instructions for MillionTrees

# GitHub Copilot Instructions

## Coding Philosophy

### Code Style Preferences
- **Pythonic**: Write idiomatic Python code that follows PEP 8 and Python best practices
- **Concise**: Favor brevity and clarity over verbosity
- **Minimal**: Use the simplest solution that works; avoid over-engineering
- **Functional**: Prefer functional programming patterns where appropriate

### Error Handling Philosophy
- **Fail Fast**: Let code fail quickly and visibly rather than hiding errors
- **Minimal Try/Except**: Avoid excessive try/except blocks that mask underlying issues
- **Explicit Failures**: When errors occur, they should be obvious and informative
- **No Silent Failures**: Don't catch exceptions unless you can meaningfully handle them

### Testing Strategy
- **Minimal Testing**: Write only essential tests that catch critical functionality
- **Quality over Quantity**: Focus on meaningful tests rather than high coverage numbers
- **Integration over Unit**: Prefer integration tests that test real workflows
- **Avoid Test Bloat**: Don't write tests for trivial getters/setters or obvious functionality

## Code Generation Guidelines

### What TO Do
- Use list/dict comprehensions instead of explicit loops when clearer
- Leverage Python's built-in functions and standard library
- Use type hints for function signatures
- Write docstrings for public functions and classes
- Use pathlib instead of os.path for file operations
- Prefer f-strings over .format() or % formatting
- Use context managers (with statements) for resource management
- Handle edge cases with early returns rather than nested conditions

### What NOT To Do
- Don't wrap every operation in try/except "just in case"
- Don't write defensive code for inputs that should be validated elsewhere
- Don't add unnecessary abstraction layers
- Don't write tests for trivial operations
- Don't catch broad exceptions (Exception, BaseException) unless absolutely necessary
- Don't suppress errors with pass statements in except blocks
- Don't add configuration options for things that don't need to be configurable

## Specific Patterns

### Preferred Error Handling
```python
# Good: Let it fail fast
def process_file(filepath):
    with open(filepath) as f:  # FileNotFoundError will bubble up naturally
        return json.load(f)    # JSONDecodeError will bubble up naturally

# Avoid: Excessive defensive programming
def process_file(filepath):
    try:
        if not os.path.exists(filepath):
            # Handle missing file
        with open(filepath) as f:
            try:
                return json.load(f)
            except JSONDecodeError:
                # Handle invalid JSON
    except Exception as e:
        # Handle everything else

Preferred Function Design

# Good: Simple, direct, functional
def filter_valid_annotations(annotations: list[dict]) -> list[dict]:
    return [ann for ann in annotations if ann.get('x') and ann.get('y')]

# Avoid: Over-engineered with unnecessary error handling
def filter_valid_annotations(annotations: list[dict]) -> list[dict]:
    try:
        if not annotations:
            return []
        result = []
        for ann in annotations:
            try:
                if ann.get('x') is not None and ann.get('y') is not None:
                    result.append(ann)
            except (KeyError, AttributeError, TypeError):
                continue
        return result
    except Exception:
        return []

Domain-Specific Guidelines

For Data Processing Scripts

  • Use pandas operations instead of manual loops
  • Leverage numpy vectorization
  • Use pathlib for file path operations
  • Let pandas/numpy raise their own exceptions rather than catching them

For CLI Scripts

  • Use argparse for command-line interfaces
  • Let missing required arguments fail naturally
  • Use sys.exit() for expected failure cases
  • Don't catch keyboard interrupts unless necessary

For API Interactions

  • Use requests library directly without excessive retry logic
  • Let HTTP errors bubble up unless you can meaningfully handle them
  • Use requests.Session for connection pooling when making multiple requests

Comments and Documentation

When to Document

  • Complex algorithms or business logic
  • Non-obvious performance considerations
  • API contracts and expected input/output formats
  • Workarounds for external library limitations

When NOT to Document

  • Obvious operations (x = x + 1)
  • Standard library usage
  • Simple getter/setter methods
  • Code that is self-explanatory

Summary

Write code that is clear, direct, and fails obviously when something goes wrong. Prefer simplicity over robustness, and trust that proper system design and monitoring will catch issues rather than trying to handle every possible error case in the code itself.

File I/O Patterns

Preferred File Reading

# Good: Let file operations fail fast and explicitly
def process_annotations(data_dir: str) -> pd.DataFrame:
    treetops_file = os.path.join(data_dir, 'treetops.gpkg')
    return gpd.read_file(treetops_file)  # FileNotFoundError will bubble up clearly

# Good: Use optional return types when files may legitimately not exist
def load_optional_config(config_path: str) -> Optional[dict]:
    try:
        with open(config_path) as f:
            return json.load(f)
    except FileNotFoundError:
        return None  # Explicit handling of expected case

# Avoid: Silent failures with existence checks
def process_annotations(data_dir: str) -> Optional[pd.DataFrame]:
    treetops_file = os.path.join(data_dir, 'treetops.gpkg')
    if os.path.exists(treetops_file):
        return gpd.read_file(treetops_file)
    return None  # Silent failure makes debugging harder

@jveitchmichaelis
Copy link
Collaborator Author

For reference on agents.md see here

@bw4sz
Copy link
Collaborator

bw4sz commented Nov 4, 2025

Let's add, 'always run test using uv sync --dev to get access to pytest and inspect the results', then I'd like to get this merged.

@jveitchmichaelis
Copy link
Collaborator Author

jveitchmichaelis commented Nov 4, 2025

Moved the files to AGENTS.md. @bw4sz I added general install instructions (uv sync --all-extras --dev) and a hint to run pre-commit defensively when finished, even if not necessarily making a commit.

@jveitchmichaelis jveitchmichaelis changed the title add copilot instructions add AGENTS.md rules Nov 6, 2025
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